diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index 316cca4f14..c7a1b291dd 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -263,6 +263,67 @@ fn get_original_name(renamed_key: &str, field_renames: &Option) -> String .unwrap_or_else(|| renamed_key.to_string()) } +/// Reject path parameter values that could alter the URL structure of the +/// internal backend request (path traversal, query/fragment injection, +/// percent-encoded and backslash bypasses). +/// +/// MCP endpoint tools build internal API URLs by string-substituting these +/// values into a fixed path template. A value containing `..` segments would +/// let a narrowly-scoped tool reach unrelated same-method endpoints once the +/// HTTP client normalizes the URL (e.g. `scripts/get/p/../../../resources/...` +/// collapses to `resources/...`). +/// +/// Only structural escapes are rejected — not every character the backend +/// happens not to use. Windmill paths legitimately contain spaces (app paths) +/// and `@` (email-style usernames, e.g. `u/admin@windmill.dev/...`); those are +/// ordinary path-segment data in an absolute URL and cannot redirect the +/// request, so rejecting them would regress valid MCP calls. +fn validate_path_param_value(param_name: &str, value: &str) -> BackendResult<()> { + let reject = |reason: &str| { + tracing::warn!( + "Rejected MCP endpoint path parameter '{}': {}", + param_name, + reason + ); + Err(ErrorData::invalid_params( + format!("Invalid path parameter '{}': {}", param_name, reason), + None, + )) + }; + + if value.is_empty() { + return reject("must not be empty"); + } + + // Structurally dangerous characters only: + // - control chars (incl. tab/CR/LF): the WHATWG URL parser strips these, + // so `..` could be reassembled into `..` + // - `\`: WHATWG converts it to `/` for http(s), enabling `..\..\` traversal + // - `%`: would let `%2e%2e%2f` decode to `../` server-side + // - `?` / `#`: query/fragment delimiters that truncate or redirect the path + // A literal space is *not* rejected: the URL crate percent-encodes it + // (`%20`) so it cannot alter routing, and app paths legitimately use it. + if let Some(bad) = value + .chars() + .find(|c| c.is_control() || matches!(*c, '\\' | '%' | '?' | '#')) + { + return reject(&format!("contains disallowed character {:?}", bad)); + } + + // No leading/trailing slash, no empty/dot/dot-dot segments. Splitting on + // `/` keeps legitimate Windmill paths (`u/alice/db`, `f/folder/name`) + // valid while catching `..`, `.`, `//`, leading and trailing `/`. + for segment in value.split('/') { + match segment { + "" => return reject("contains an empty path segment or leading/trailing slash"), + "." | ".." => return reject("contains a '.' or '..' path segment"), + _ => {} + } + } + + Ok(()) +} + /// Substitute path parameters in the URL template pub fn substitute_path_params( path: &str, @@ -282,6 +343,7 @@ pub fn substitute_path_params( match args_map.get(param_name) { Some(param_value) => { if let Some(str_val) = param_value.as_str() { + validate_path_param_value(&original_name, str_val)?; path_template = path_template.replace(&placeholder, str_val); } } @@ -442,3 +504,100 @@ pub async fn parse_response_body(response: Response) -> BackendResult '/' + "with\nnewline", // control char (stripped by URL parser) + "tab\there", // control char + "query?x=1", // query delimiter truncates the path + "frag#ment", // fragment delimiter truncates the path + "pct%2e%2e%2fencoded", // percent-encoded `../` + ] { + assert!( + validate_path_param_value("path", bad).is_err(), + "expected {bad:?} to be rejected" + ); + } + } + + #[test] + fn substitute_path_params_blocks_cross_endpoint_traversal() { + let path_schema = Some(json!({ + "type": "object", + "properties": { "path": { "type": "string" } } + })); + let mut args = serde_json::Map::new(); + args.insert( + "path".to_string(), + json!("../../../resources/get/u/alice/prod_db"), + ); + + let result = substitute_path_params( + "/w/{workspace}/scripts/get/p/{path}", + "dev", + &args, + &path_schema, + &None, + ); + assert!( + result.is_err(), + "traversal payload must be rejected before URL substitution" + ); + } + + #[test] + fn substitute_path_params_allows_normal_path() { + let path_schema = Some(json!({ + "type": "object", + "properties": { "path": { "type": "string" } } + })); + let mut args = serde_json::Map::new(); + args.insert("path".to_string(), json!("u/alice/my_script")); + + let result = substitute_path_params( + "/w/{workspace}/scripts/get/p/{path}", + "dev", + &args, + &path_schema, + &None, + ) + .expect("legitimate path should substitute"); + assert_eq!(result, "/w/dev/scripts/get/p/u/alice/my_script"); + } +}