fix: reject path traversal in MCP endpoint path parameters (#9211)

* fix: reject path traversal in MCP endpoint path parameters

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: narrow MCP path-param validator to structural escapes only

Codex review: rejecting whitespace/`:`/`@` regressed legitimate
Windmill paths (app paths with spaces, email-style usernames like
u/admin@windmill.dev/...). These are ordinary path-segment data in an
absolute URL and cannot redirect the request. Reject only structural
escapes: control chars, `\`, `%`, `?`, `#`, and `.`/`..`/empty segments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

backend/windmill-api/src/mcp/utils.rs

@@ -263,6 +263,67 @@ fn get_original_name(renamed_key: &str, field_renames: &Option<Value>) -> 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 `.<TAB>.` 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<Body>) -> BackendResult<Valu
 
     Ok(serde_json::from_str(&body_str).unwrap_or_else(|_| Value::String(body_str)))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serde_json::json;
+
+    #[test]
+    fn validate_path_param_value_accepts_legitimate_windmill_paths() {
+        for ok in [
+            "u/alice/prod_db",
+            "f/folder/sub/my-script",
+            "g/all",
+            "myscript",
+            "01h00000-0000-0000-0000-000000000000",
+            "123",
+            "u/admin/My App",         // app paths legitimately contain spaces
+            "u/admin@windmill.dev/x", // email-style usernames contain '@'
+            "f/folder/tag:v1",        // ':' is valid path-segment data
+        ] {
+            assert!(
+                validate_path_param_value("path", ok).is_ok(),
+                "expected {ok:?} to be accepted"
+            );
+        }
+    }
+
+    #[test]
+    fn validate_path_param_value_rejects_traversal_and_injection() {
+        for bad in [
+            "../../../resources/get/u/alice/prod_db", // path traversal (the report PoC)
+            "..",
+            ".",
+            "a/../b",
+            "a/./b",
+            "/leading",
+            "trailing/",
+            "double//slash",
+            "",
+            "back\\slash",         // WHATWG converts '\' -> '/'
+            "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");
+    }
+}