mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-08-19 00:02:03 +00:00
fix(mcp): stop double-escaping string query params in build_query_string (#9855)
MCP tool arguments were converted to URL query values via `value.to_string()`
+ `trim_matches('"')`. For string values containing JSON (e.g. the `args`/`result`
filters on job listing, `args` on schedule listing), `to_string()` JSON-encodes the
string and escapes inner quotes with backslashes; stripping the outer quotes leaves
`{\"k\":\"v\"}`, which the backend's `serde_json::from_str` then fails to parse,
falling back to `FALSE` and returning zero results.
Use `value.as_str()` to emit the raw string content for `Value::String`, falling
back to `value.to_string()` for non-string types (numbers, booleans). Adds
regression tests covering JSON-string, non-string, and plain-string params.
Fixes WIN-2114
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -382,12 +382,18 @@ pub fn build_query_string(
|
||||
.map(|value| {
|
||||
// Use the original name for the query parameter key
|
||||
let original_name = get_original_name(param_name, query_field_renames);
|
||||
let value_str = value.to_string();
|
||||
let str_val = value_str.trim_matches('"');
|
||||
// For string values, use the raw content: to_string() would JSON-encode
|
||||
// it, and stripping the outer quotes leaves inner quotes backslash-escaped
|
||||
// (e.g. `{\"k\":\"v\"}`), which breaks downstream JSON parsing of params
|
||||
// like `args`/`result`. Non-string values keep their JSON serialization.
|
||||
let str_val = value
|
||||
.as_str()
|
||||
.map(|s| s.to_string())
|
||||
.unwrap_or_else(|| value.to_string());
|
||||
format!(
|
||||
"{}={}",
|
||||
urlencoding::encode(&original_name),
|
||||
urlencoding::encode(str_val)
|
||||
urlencoding::encode(&str_val)
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -624,4 +630,55 @@ mod tests {
|
||||
.expect("legitimate path should substitute");
|
||||
assert_eq!(result, "/w/dev/scripts/get/p/u/alice/my_script");
|
||||
}
|
||||
|
||||
fn single_query_schema(param: &str) -> Option<Value> {
|
||||
Some(json!({
|
||||
"type": "object",
|
||||
"properties": { param: { "type": "string" } }
|
||||
}))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_query_string_preserves_json_string_content() {
|
||||
// A string param carrying JSON (e.g. the `args` filter on listJobs) must be
|
||||
// emitted as its raw content so the backend can `serde_json::from_str` it.
|
||||
let mut args = serde_json::Map::new();
|
||||
args.insert("args".to_string(), json!("{\"key\":\"val\"}"));
|
||||
|
||||
let qs = build_query_string(&args, &single_query_schema("args"), &None);
|
||||
|
||||
// No backslash escaping: %5C must not appear; the encoded braces/quotes are exact.
|
||||
assert_eq!(qs, "?args=%7B%22key%22%3A%22val%22%7D");
|
||||
assert!(
|
||||
!qs.contains("%5C"),
|
||||
"must not contain backslash escapes: {qs}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_query_string_keeps_non_string_serialization() {
|
||||
let mut args = serde_json::Map::new();
|
||||
args.insert("per_page".to_string(), json!(42));
|
||||
assert_eq!(
|
||||
build_query_string(&args, &single_query_schema("per_page"), &None),
|
||||
"?per_page=42"
|
||||
);
|
||||
|
||||
let mut args = serde_json::Map::new();
|
||||
args.insert("running".to_string(), json!(true));
|
||||
assert_eq!(
|
||||
build_query_string(&args, &single_query_schema("running"), &None),
|
||||
"?running=true"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_query_string_encodes_plain_string() {
|
||||
let mut args = serde_json::Map::new();
|
||||
args.insert("path".to_string(), json!("u/alice/my script"));
|
||||
assert_eq!(
|
||||
build_query_string(&args, &single_query_schema("path"), &None),
|
||||
"?path=u%2Falice%2Fmy%20script"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user