starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-08-20 16:02:19 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: refuse an MCP endpoint call whose required request body is empty (#10771)

Browse Source 
* fix: refuse an MCP endpoint call whose required request body is empty

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs: state the required-body rationale once

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
hugocasa
2026-08-20 15:40:57 +02:00
committed by GitHub
parent 1f59841a67
commit dad8fed647
8 changed files with 298 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/generate_mcp_endpoints_tools/generate_mcp_tools.py
+8 -1
View File
@@ -147,7 +147,14 @@ def extract_separate_schemas(parameters: List[Dict[str, Any]], request_body: Opt
else:
# Log warning when a required field is missing from schema properties
print(f"Warning: Required field '{field}' not found in body schema properties", file=sys.stderr)
# Marks a body the MCP layer must refuse to leave empty (see non_empty_body_fields
# in windmill-mcp). A pass-through body (no declared properties, e.g.
# runScriptByPath) is excluded: `{}` is a valid body there, for a runnable that
# takes no arguments.
if body_schema and request_body.get('required') and body_schema.get('properties'):
body_schema['minProperties'] = 1
# Sanitize empty schemas for JSON Schema draft 2020-12 compliance
path_params_schema = sanitize_empty_schemas(path_params_schema)
query_params_schema = sanitize_empty_schemas(query_params_schema)
backend/windmill-api/src/mcp/auto_generated_endpoints.rs
+24 -12
View File
@@ -162,7 +162,8 @@ pub fn all_tools() -> Vec<EndpointTool> {
"value",
"is_secret",
"description"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -244,7 +245,8 @@ pub fn all_tools() -> Vec<EndpointTool> {
"type": "string",
"description": "The path to the variable (body parameter). Defaults to `path` when omitted; set it only to change the path."
}
}
},
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: Some(serde_json::json!({
@@ -392,7 +394,8 @@ pub fn all_tools() -> Vec<EndpointTool> {
"path",
"value",
"resource_type"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -464,7 +467,8 @@ pub fn all_tools() -> Vec<EndpointTool> {
"type": "string",
"description": "The path to the resource (body parameter). Defaults to `path` when omitted; set it only to change the path."
}
}
},
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: Some(serde_json::json!({
@@ -719,7 +723,8 @@ Creates a new version of an existing script when called with the same path and t
"summary",
"content",
"language"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -980,7 +985,8 @@ Creates a new version of an existing script when called with the same path and t
"value",
"path"
],
"description": "Top-level flow definition containing metadata, configuration, and the flow structure"
"description": "Top-level flow definition containing metadata, configuration, and the flow structure",
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -1035,7 +1041,8 @@ Creates a new version of an existing script when called with the same path and t
"summary",
"value"
],
"description": "Top-level flow definition containing metadata, configuration, and the flow structure"
"description": "Top-level flow definition containing metadata, configuration, and the flow structure",
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: Some(serde_json::json!({
@@ -1226,7 +1233,8 @@ Creates a new version of an existing script when called with the same path and t
"value",
"summary",
"policy"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -1341,7 +1349,8 @@ Creates a new version of an existing script when called with the same path and t
},
"required": [
"value"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: Some(serde_json::json!({
@@ -1463,7 +1472,8 @@ Creates a new version of an existing script when called with the same path and t
"args",
"content",
"language"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -2034,7 +2044,8 @@ You should get the schema of the script or flow before creating the schedule to
"script_path",
"is_flow",
"args"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
@@ -2239,7 +2250,8 @@ You should get the schema of the script or flow before updating the schedule to
"schedule",
"timezone",
"args"
]
],
"minProperties": 1
})),
query_field_renames: None,
body_field_renames: None,
backend/windmill-api/src/mcp/core.rs
+1 -8
View File
@@ -291,14 +291,7 @@ impl McpBackend for WindmillBackend {
);
// Prepare request body
let body_json = build_request_body(
&endpoint_tool.method,
args_map,
&endpoint_tool.body_schema,
&endpoint_tool.body_field_renames,
&endpoint_tool.path_params_schema,
&endpoint_tool.query_params_schema,
);
let body_json = build_request_body(endpoint_tool, args_map)?;
// Create and execute request
let response = create_http_request(
backend/windmill-api/src/mcp/utils.rs
+155 -55
View File
@@ -15,7 +15,9 @@ use windmill_common::scripts::{get_full_hub_script_by_path, Schema};
use windmill_common::utils::{query_elems_from_hub, StripPath};
use windmill_common::worker::to_raw_value;
use windmill_common::{DB, HUB_BASE_URL};
use windmill_mcp::server::{BackendResult, ErrorData, PathFilter};
use windmill_mcp::server::{
non_empty_body_fields, BackendResult, EndpointTool, ErrorData, PathFilter,
};
use windmill_mcp::{HubResponse, HubScriptInfo, ItemSchema, ResourceInfo, ResourceType};
use crate::db::ApiAuthed;
@@ -448,15 +450,44 @@ pub fn build_query_string(
}
}
/// Build request body from arguments
/// Build request body from arguments, refusing a call that would reach an
/// endpoint requiring a body without one.
pub fn build_request_body(
method: &str,
tool: &EndpointTool,
args_map: &serde_json::Map<String, Value>,
) -> BackendResult<Option<Value>> {
let body = assemble_request_body(tool, args_map);
// The fields that would have satisfied it are not expressible in the tool's input
// schema, so name them here rather than dispatching a call that cannot succeed.
if body.is_none() {
if let Some(fields) = non_empty_body_fields(tool) {
return Err(ErrorData::invalid_params(
format!(
"{} needs a request body: provide at least one of {}",
tool.name,
fields.join(", ")
),
None,
));
}
}
Ok(body)
}
fn assemble_request_body(
tool: &EndpointTool,
args_map: &serde_json::Map<String, Value>,
body_schema: &Option<Value>,
body_field_renames: &Option<Value>,
path_params_schema: &Option<Value>,
query_params_schema: &Option<Value>,
) -> Option<Value> {
let EndpointTool {
method,
body_schema,
body_field_renames,
path_params_schema,
query_params_schema,
..
} = tool;
if method == "GET" {
return None;
}
@@ -805,27 +836,64 @@ mod tests {
);
}
fn endpoint_tool(
method: &'static str,
path_params_schema: Option<Value>,
body_schema: Option<Value>,
body_field_renames: Option<Value>,
) -> EndpointTool {
EndpointTool {
name: std::borrow::Cow::Borrowed("testTool"),
description: std::borrow::Cow::Borrowed("desc"),
instructions: std::borrow::Cow::Borrowed(""),
path: std::borrow::Cow::Borrowed("/w/{workspace}/test/{path}"),
method: std::borrow::Cow::Borrowed(method),
path_params_schema,
query_params_schema: None,
body_schema,
query_field_renames: None,
body_field_renames,
}
}
fn args_of(value: Value) -> serde_json::Map<String, Value> {
value.as_object().unwrap().clone()
}
fn path_param_schema() -> Option<Value> {
Some(json!({
"type": "object",
"properties": { "path": { "type": "string" } },
"required": ["path"]
}))
}
fn generated_tool(name: &str) -> EndpointTool {
crate::mcp::auto_generated_endpoints::all_tools()
.into_iter()
.find(|t| t.name.as_ref() == name)
.unwrap_or_else(|| panic!("{name} must be a generated endpoint tool"))
}
#[test]
fn build_request_body_passthrough_forwards_script_args_minus_path() {
// runScriptByPath-shaped body: additionalProperties, no declared props.
// `path` is a path param and must be excluded; the rest are the script's
// arguments and must be forwarded verbatim.
let body_schema = Some(json!({ "type": "object", "additionalProperties": true }));
let path_schema = Some(json!({
"type": "object",
"properties": { "path": { "type": "string" } },
"required": ["path"]
}));
let args: serde_json::Map<String, Value> = json!({
let tool = endpoint_tool(
"POST",
path_param_schema(),
Some(json!({ "type": "object", "additionalProperties": true })),
None,
);
let args = args_of(json!({
"path": "u/admin/my_script",
"name": "alice",
"count": 3
})
.as_object()
.unwrap()
.clone();
}));
let body = build_request_body("POST", &args, &body_schema, &None, &path_schema, &None)
let body = build_request_body(&tool, &args)
.unwrap()
.expect("passthrough body should be built");
let obj = body.as_object().unwrap();
assert_eq!(obj.get("name"), Some(&json!("alice")));
@@ -839,16 +907,18 @@ mod tests {
#[test]
fn build_request_body_declared_props_only_forwards_declared() {
// Endpoints with explicit properties keep the strict declared-only behavior.
let body_schema = Some(json!({
"type": "object",
"properties": { "value": { "type": "string" } },
"required": ["value"]
}));
let args: serde_json::Map<String, Value> = json!({ "value": "x", "sneaky": "y" })
.as_object()
.unwrap()
.clone();
let body = build_request_body("POST", &args, &body_schema, &None, &None, &None).unwrap();
let tool = endpoint_tool(
"POST",
None,
Some(json!({
"type": "object",
"properties": { "value": { "type": "string" } },
"required": ["value"]
})),
None,
);
let args = args_of(json!({ "value": "x", "sneaky": "y" }));
let body = build_request_body(&tool, &args).unwrap().unwrap();
let obj = body.as_object().unwrap();
assert_eq!(obj.get("value"), Some(&json!("x")));
assert!(
@@ -859,13 +929,10 @@ mod tests {
// updateFlow-shaped: `path` is both a path parameter and a body field. The path
// parameter keeps the plain name; only the body side is mangled.
fn update_flow_schemas() -> (Option<Value>, Option<Value>, Option<Value>) {
(
Some(json!({
"type": "object",
"properties": { "path": { "type": "string" } },
"required": ["path"]
})),
fn update_flow_tool() -> EndpointTool {
endpoint_tool(
"POST",
path_param_schema(),
Some(json!({
"type": "object",
"properties": {
@@ -873,7 +940,8 @@ mod tests {
"value": { "type": "object" },
"path__body": { "type": "string" }
},
"required": ["summary", "value"]
"required": ["summary", "value"],
"minProperties": 1
})),
Some(json!({ "path__body": "path" })),
)
@@ -884,26 +952,16 @@ mod tests {
// The mangled body field carries the *new* path when renaming; it must reach the
// API under its original name `path`. (An omitted `path__body` is intentionally
// absent from the body; the server defaults it from the URL path parameter.)
let (path_schema, body_schema, body_renames) = update_flow_schemas();
let args: serde_json::Map<String, Value> = json!({
let args = args_of(json!({
"path": "f/team/my_flow",
"path__body": "f/team/renamed_flow",
"summary": "s",
"value": {}
})
.as_object()
.unwrap()
.clone();
}));
let body = build_request_body(
"POST",
&args,
&body_schema,
&body_renames,
&path_schema,
&None,
)
.expect("body should be built");
let body = build_request_body(&update_flow_tool(), &args)
.unwrap()
.expect("body should be built");
assert_eq!(
body.as_object().unwrap().get("path"),
Some(&json!("f/team/renamed_flow")),
@@ -913,9 +971,51 @@ mod tests {
#[test]
fn build_request_body_get_has_no_body() {
let body_schema = Some(json!({ "type": "object", "additionalProperties": true }));
let args: serde_json::Map<String, Value> = json!({ "a": 1 }).as_object().unwrap().clone();
assert!(build_request_body("GET", &args, &body_schema, &None, &None, &None).is_none());
let tool = endpoint_tool(
"GET",
None,
Some(json!({ "type": "object", "additionalProperties": true })),
None,
);
assert!(build_request_body(&tool, &args_of(json!({ "a": 1 })))
.unwrap()
.is_none());
}
#[test]
fn build_request_body_refuses_bodyless_call_to_required_body_endpoint() {
// updateVariable's only required argument is the path parameter, so a
// path-only call satisfies the tool's input schema while leaving the body
// empty. Dispatching it would reach the API with no JSON body and come back
// as a 415, so it is refused here with the fields it could have set.
let tool = generated_tool("updateVariable");
let err = build_request_body(&tool, &args_of(json!({ "path": "u/admin/a_var" })))
.expect_err("a path-only updateVariable call must be refused");
assert!(
err.message.contains("value"),
"the error must name the body fields, got: {}",
err.message
);
let body = build_request_body(
&tool,
&args_of(json!({ "path": "u/admin/a_var", "value": "v" })),
)
.unwrap()
.expect("a call carrying an update must still build a body");
assert_eq!(body.as_object().unwrap().get("value"), Some(&json!("v")));
}
#[test]
fn build_request_body_allows_bodyless_run_of_a_no_arg_runnable() {
// The counterpart: a runnable that takes no arguments has nothing to put in
// the body, and the run endpoints accept an empty one.
let tool = generated_tool("runScriptByPath");
assert!(
build_request_body(&tool, &args_of(json!({ "path": "u/admin/no_args"})))
.unwrap()
.is_none()
);
}
#[test]
backend/windmill-mcp/src/server/endpoints.rs
+80 -1
View File
@@ -31,6 +31,22 @@ pub fn is_endpoint_read_only(tool: &EndpointTool) -> bool {
tool.method.as_ref() == "GET"
}
/// The body fields of an endpoint that cannot be called with an empty body, or
/// `None` when a bodyless call is legitimate.
///
/// `minProperties` on the body schema marks an operation whose OpenAPI declares
/// `requestBody: required: true` (see `generate_mcp_tools.py`). The API answers a
/// request carrying no JSON body with 415 before the handler runs, so a call
/// filling none of these fields can only fail.
pub fn non_empty_body_fields(tool: &EndpointTool) -> Option<Vec<&str>> {
let schema = tool.body_schema.as_ref()?;
if schema.get("minProperties").and_then(|m| m.as_u64())? == 0 {
return None;
}
let props = schema.get("properties")?.as_object()?;
Some(props.keys().map(|k| k.as_str()).collect())
}
/// Convert a single endpoint tool to MCP tool
pub fn endpoint_tool_to_mcp_tool(tool: &EndpointTool) -> Tool {
let mut combined_properties = serde_json::Map::new();
@@ -54,7 +70,23 @@ pub fn endpoint_tool_to_mcp_tool(tool: &EndpointTool) -> Tool {
});
make_schema_compatible(&mut combined_schema);
let description = format!("{}. {}", tool.description, tool.instructions);
let mut description = format!("{}. {}", tool.description, tool.instructions);
// A body that must not be empty, none of whose fields is individually required,
// is a requirement no `required` array can express. Spell it out, or the schema
// reads as if the path alone were a complete call.
if let Some(fields) = non_empty_body_fields(tool) {
if !fields
.iter()
.any(|f| combined_required.iter().any(|r| r == f))
{
description = format!(
"{} At least one of these arguments must be provided: {}.",
description.trim_end(),
fields.join(", ")
);
}
}
// Create annotations based on HTTP method and endpoint characteristics
let annotations = create_endpoint_annotations(tool);
@@ -289,6 +321,53 @@ mod tests {
);
}
/// updateVariable-shaped: the body must not be empty, but no single field of it
/// is required, so `required` cannot carry the constraint and the prose must.
fn update_tool(body_required: &[&str]) -> EndpointTool {
let mut t = tool("updateVariable", "/w/{workspace}/variables/update/{path}");
t.method = Cow::Borrowed("POST");
t.path_params_schema = Some(serde_json::json!({
"type": "object",
"properties": { "path": { "type": "string" } },
"required": ["path"]
}));
t.body_schema = Some(serde_json::json!({
"type": "object",
"properties": {
"value": { "type": "string" },
"is_secret": { "type": "boolean" }
},
"required": body_required,
"minProperties": 1
}));
t
}
#[test]
fn required_body_with_no_required_field_is_stated_in_the_description() {
let desc = endpoint_tool_to_mcp_tool(&update_tool(&[]))
.description
.unwrap_or_default()
.to_string();
assert!(
desc.contains("value, is_secret"),
"the description must name the body fields, got: {desc}"
);
}
#[test]
fn required_body_with_a_required_field_keeps_its_description() {
// `required` already forbids the empty body here, so the sentence would be noise.
let desc = endpoint_tool_to_mcp_tool(&update_tool(&["value"]))
.description
.unwrap_or_default()
.to_string();
assert!(
!desc.contains("At least one"),
"no extra sentence is needed when a body field is required, got: {desc}"
);
}
#[test]
fn list_workspaces_tool_has_no_params() {
let t = list_workspaces_tool();
backend/windmill-mcp/src/server/mod.rs
+1 -1
View File
@@ -15,7 +15,7 @@ pub use crate::common::types::{McpToken, MultiWorkspaceMcp, WorkspaceInfo};
pub use backend::{BackendResult, McpAuth, McpBackend, PathFilter};
pub use endpoints::{
endpoint_tool_to_mcp_tool, endpoint_tool_to_mcp_tool_multi, is_endpoint_read_only,
list_workspaces_tool, EndpointTool,
list_workspaces_tool, non_empty_body_fields, EndpointTool,
};
pub use runner::Runner;
pub use tools::create_tool_from_item;
backend/windmill-mcp/src/server/runner.rs
+5 -5
View File
@@ -607,12 +607,13 @@ impl<B: McpBackend> Runner<B> {
// including the run-by-path path check (shared with multi mode).
authorize_endpoint_call(scope_config, endpoint_tool, &args, read_only)?;
// This is an endpoint tool, call via backend
// This is an endpoint tool, call via backend. The backend's own error
// code is kept: a client that retries an internal error would loop on
// arguments the endpoint rejected.
let result = self
.backend
.call_endpoint(auth, workspace_id, endpoint_tool, args)
.await
.map_err(|e| ErrorData::internal_error(e.message, None))?;
.await?;
return Ok(CallToolResult::success(vec![ContentBlock::text(
truncate_tool_result(
@@ -865,8 +866,7 @@ impl<B: McpBackend> Runner<B> {
let result = self
.backend
.call_endpoint(&resolved_auth, &workspace_id, endpoint_tool, args)
.await
.map_err(|e| ErrorData::internal_error(e.message, None))?;
.await?;
Ok(CallToolResult::success(vec![ContentBlock::text(
truncate_tool_result(
frontend/src/lib/mcpEndpointTools.ts
+24 -12
View File
@@ -171,7 +171,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"value",
"is_secret",
"description"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -253,7 +254,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"type": "string",
"description": "The path to the variable (body parameter). Defaults to `path` when omitted; set it only to change the path."
}
}
},
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: {
@@ -401,7 +403,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"path",
"value",
"resource_type"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -473,7 +476,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"type": "string",
"description": "The path to the resource (body parameter). Defaults to `path` when omitted; set it only to change the path."
}
}
},
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: {
@@ -727,7 +731,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"summary",
"content",
"language"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -988,7 +993,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"value",
"path"
],
"description": "Top-level flow definition containing metadata, configuration, and the flow structure"
"description": "Top-level flow definition containing metadata, configuration, and the flow structure",
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -1043,7 +1049,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"summary",
"value"
],
"description": "Top-level flow definition containing metadata, configuration, and the flow structure"
"description": "Top-level flow definition containing metadata, configuration, and the flow structure",
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: {
@@ -1234,7 +1241,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"value",
"summary",
"policy"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -1349,7 +1357,8 @@ export const mcpEndpointTools: EndpointTool[] = [
},
"required": [
"value"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: {
@@ -1471,7 +1480,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"args",
"content",
"language"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -2039,7 +2049,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"script_path",
"is_flow",
"args"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined
@@ -2241,7 +2252,8 @@ export const mcpEndpointTools: EndpointTool[] = [
"schedule",
"timezone",
"args"
]
],
"minProperties": 1
},
queryFieldRenames: undefined,
bodyFieldRenames: undefined