From 8df51c04ae8cfa8fbf71240e9aaf23c70f34fbca Mon Sep 17 00:00:00 2001 From: shiben <807629978@qq.com> Date: Fri, 13 Mar 2026 02:38:30 +0800 Subject: [PATCH] feat: enhance HTTP tool parameter parsing (#911) * feat: enhance HTTP tool parameter parsing - Add support for stringified JSON arrays in headers parameter. - Introduce timeout_secs parameter parsing to accept both numbers and string representations. - Implement save_to parameter parsing to handle empty strings as None. - Update HTTP request handling to incorporate timeout and save_to parameters. - Add unit tests for new parsing functions to ensure correct behavior. * feat(http): enhance HTTP tool with timeout and header parsing improvements - Introduced default and maximum request timeout constants to manage resource usage. - Refactored header parsing logic to separate functions for better readability and maintainability. - Updated timeout handling to ensure it respects the maximum allowed value. - Added unit tests to validate new header parsing functionality. * refactor(http): replace hardcoded timeout with effective_timeout variable in HTTP tool error handling --- src/tools/builtin/http.rs | 247 ++++++++++++++++++++++++++++++++------ 1 file changed, 211 insertions(+), 36 deletions(-) diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index b1b1994d..e8138a26 100644 --- a/src/tools/builtin/http.rs +++ b/src/tools/builtin/http.rs @@ -31,6 +31,12 @@ const MAX_RESPONSE_SIZE: usize = 5 * 1024 * 1024; /// in memory for LLM context. Matches the WASM attachment size cap. const MAX_SAVE_TO_SIZE: usize = 50 * 1024 * 1024; +/// Default request timeout when the caller does not provide one. +const DEFAULT_TIMEOUT_SECS: u64 = 30; + +/// Maximum allowed request timeout to bound resource usage from LLM-controlled inputs. +const MAX_TIMEOUT_SECS: u64 = 300; + /// Maximum number of redirects to follow for simple GET requests. const MAX_REDIRECTS: usize = 3; @@ -244,43 +250,120 @@ fn is_html_response(headers: &HashMap) -> bool { fn parse_headers_param( headers: Option<&serde_json::Value>, ) -> Result, ToolError> { + fn parse_header_object( + map: &serde_json::Map, + ) -> Result, ToolError> { + let mut out = Vec::with_capacity(map.len()); + for (k, v) in map { + let value = v.as_str().ok_or_else(|| { + ToolError::InvalidParameters(format!("header '{}' must have a string value", k)) + })?; + out.push((k.clone(), value.to_string())); + } + Ok(out) + } + + fn parse_header_array(items: &[serde_json::Value]) -> Result, ToolError> { + let mut out = Vec::with_capacity(items.len()); + for (idx, item) in items.iter().enumerate() { + let obj = item.as_object().ok_or_else(|| { + ToolError::InvalidParameters(format!( + "headers[{}] must be an object with 'name' and 'value'", + idx + )) + })?; + let name = obj.get("name").and_then(|v| v.as_str()).ok_or_else(|| { + ToolError::InvalidParameters(format!("headers[{}].name must be a string", idx)) + })?; + let value = obj.get("value").and_then(|v| v.as_str()).ok_or_else(|| { + ToolError::InvalidParameters(format!("headers[{}].value must be a string", idx)) + })?; + out.push((name.to_string(), value.to_string())); + } + Ok(out) + } + match headers { None => Ok(Vec::new()), - Some(serde_json::Value::Object(map)) => { - let mut out = Vec::with_capacity(map.len()); - for (k, v) in map { - let value = v.as_str().ok_or_else(|| { - ToolError::InvalidParameters(format!("header '{}' must have a string value", k)) - })?; - out.push((k.clone(), value.to_string())); + Some(serde_json::Value::String(raw)) => { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Ok(Vec::new()); } - Ok(out) - } - Some(serde_json::Value::Array(items)) => { - let mut out = Vec::with_capacity(items.len()); - for (idx, item) in items.iter().enumerate() { - let obj = item.as_object().ok_or_else(|| { - ToolError::InvalidParameters(format!( - "headers[{}] must be an object with 'name' and 'value'", - idx - )) - })?; - let name = obj.get("name").and_then(|v| v.as_str()).ok_or_else(|| { - ToolError::InvalidParameters(format!("headers[{}].name must be a string", idx)) - })?; - let value = obj.get("value").and_then(|v| v.as_str()).ok_or_else(|| { - ToolError::InvalidParameters(format!("headers[{}].value must be a string", idx)) - })?; - out.push((name.to_string(), value.to_string())); + let parsed = serde_json::from_str::(trimmed).map_err(|e| { + ToolError::InvalidParameters(format!( + "headers string must contain valid JSON object/array: {}", + e + )) + })?; + match parsed { + serde_json::Value::Object(map) => parse_header_object(&map), + serde_json::Value::Array(items) => parse_header_array(&items), + _ => Err(ToolError::InvalidParameters( + "headers string must decode to a JSON object or array".to_string(), + )), } - Ok(out) } + Some(serde_json::Value::Object(map)) => parse_header_object(map), + Some(serde_json::Value::Array(items)) => parse_header_array(items), Some(_) => Err(ToolError::InvalidParameters( "'headers' must be an object or an array of {name, value}".to_string(), )), } } +fn parse_timeout_secs_param(timeout: Option<&serde_json::Value>) -> Result, ToolError> { + let parsed = match timeout { + None | Some(serde_json::Value::Null) => Ok(None), + Some(serde_json::Value::Number(n)) => n.as_u64().map(Some).ok_or_else(|| { + ToolError::InvalidParameters("timeout_secs must be a non-negative integer".to_string()) + }), + Some(serde_json::Value::String(raw)) => { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Ok(None); + } + let secs = trimmed.parse::().map_err(|_| { + ToolError::InvalidParameters( + "timeout_secs string must contain a non-negative integer".to_string(), + ) + })?; + Ok(Some(secs)) + } + Some(_) => Err(ToolError::InvalidParameters( + "timeout_secs must be an integer".to_string(), + )), + }?; + + if let Some(secs) = parsed + && secs > MAX_TIMEOUT_SECS + { + return Err(ToolError::InvalidParameters(format!( + "timeout_secs must be <= {}", + MAX_TIMEOUT_SECS + ))); + } + + Ok(parsed) +} + +fn parse_save_to_param(save_to: Option<&serde_json::Value>) -> Result, ToolError> { + match save_to { + None | Some(serde_json::Value::Null) => Ok(None), + Some(serde_json::Value::String(path)) => { + let trimmed = path.trim(); + if trimmed.is_empty() { + Ok(None) + } else { + Ok(Some(trimmed.to_string())) + } + } + Some(_) => Err(ToolError::InvalidParameters( + "save_to must be a string".to_string(), + )), + } +} + /// Extract host from URL in params (for approval checks). fn extract_host_from_params(params: &serde_json::Value) -> Option { params @@ -358,6 +441,7 @@ impl Tool for HttpTool { let start = std::time::Instant::now(); let method = require_str(¶ms, "method")?; + let method_upper = method.to_uppercase(); let url = require_str(¶ms, "url")?; let mut parsed_url = validate_url(url)?; @@ -379,6 +463,9 @@ impl Tool for HttpTool { // Parse headers let mut headers_vec = parse_headers_param(params.get("headers"))?; + let timeout_secs = parse_timeout_secs_param(params.get("timeout_secs"))?; + let save_to = parse_save_to_param(params.get("save_to"))?; + let effective_timeout = Duration::from_secs(timeout_secs.unwrap_or(DEFAULT_TIMEOUT_SECS)); // Build request let mut request = match method.to_uppercase().as_str() { @@ -395,6 +482,8 @@ impl Tool for HttpTool { } }; + request = request.timeout(effective_timeout); + // Add headers for (key, value) in &headers_vec { request = request.header(key.as_str(), value.as_str()); @@ -403,7 +492,9 @@ impl Tool for HttpTool { // Add body if present let body_bytes = if let Some(body) = params.get("body") { if let Some(body_str) = body.as_str() { - if let Ok(json_body) = serde_json::from_str::(body_str) { + if body_str.is_empty() { + None + } else if let Ok(json_body) = serde_json::from_str::(body_str) { let bytes = serde_json::to_vec(&json_body).map_err(|e| { ToolError::InvalidParameters(format!("invalid body JSON: {}", e)) })?; @@ -468,7 +559,7 @@ impl Tool for HttpTool { // Build the interceptor request descriptor for recording/replay let intercept_req = crate::llm::recording::HttpExchangeRequest { - method: method.to_uppercase(), + method: method_upper, url: parsed_url.to_string(), headers: headers_vec.clone(), body: body_bytes @@ -510,7 +601,7 @@ impl Tool for HttpTool { let hop_client = build_pinned_client( &hop_host, &hop_addrs, - Duration::from_secs(30), + effective_timeout, reqwest::redirect::Policy::none(), )?; @@ -524,7 +615,7 @@ impl Tool for HttpTool { .await .map_err(|e| { if e.is_timeout() { - ToolError::Timeout(Duration::from_secs(30)) + ToolError::Timeout(effective_timeout) } else { ToolError::ExternalService(e.to_string()) } @@ -588,7 +679,7 @@ impl Tool for HttpTool { } else { let resp = request.send().await.map_err(|e| { if e.is_timeout() { - ToolError::Timeout(Duration::from_secs(30)) + ToolError::Timeout(effective_timeout) } else { ToolError::ExternalService(e.to_string()) } @@ -616,7 +707,7 @@ impl Tool for HttpTool { .collect(); // Use a larger size limit when saving to disk (file downloads) - let saving_to_disk = params.get("save_to").is_some(); + let saving_to_disk = save_to.is_some(); let max_size = if saving_to_disk { MAX_SAVE_TO_SIZE } else { @@ -661,11 +752,11 @@ impl Tool for HttpTool { let body_bytes = bytes::Bytes::from(body); // If save_to is specified, write raw bytes to file and return metadata. - if let Some(save_to) = params.get("save_to").and_then(|v| v.as_str()) { - let save_to_owned = save_to.to_string(); + if let Some(save_to) = save_to { + let saved_to = save_to.clone(); let bytes_clone = body_bytes.clone(); tokio::task::spawn_blocking(move || { - let canonical = validate_save_to_path(&save_to_owned)?; + let canonical = validate_save_to_path(&save_to)?; std::fs::write(&canonical, &bytes_clone).map_err(|e| { ToolError::ExecutionFailed(format!("failed to write file: {}", e)) })?; @@ -676,7 +767,7 @@ impl Tool for HttpTool { .map_err(|e: ToolError| e)?; let result = serde_json::json!({ "status": status, - "saved_to": save_to, + "saved_to": saved_to, "size_bytes": body_bytes.len(), "headers": headers, }); @@ -887,6 +978,71 @@ mod tests { ); } + #[test] + fn test_parse_headers_param_accepts_stringified_array() { + let headers = + serde_json::json!("[{\"name\":\"Authorization\",\"value\":\"Bearer token\"}]"); + let parsed = parse_headers_param(Some(&headers)).unwrap(); + assert_eq!( + parsed, + vec![("Authorization".to_string(), "Bearer token".to_string())] + ); + } + + #[test] + fn test_parse_headers_param_rejects_double_string_encoding() { + let headers = serde_json::json!("\"hello\""); + let err = parse_headers_param(Some(&headers)).unwrap_err(); + assert!( + err.to_string() + .contains("headers string must decode to a JSON object or array"), + "unexpected error: {}", + err + ); + } + + #[test] + fn test_parse_timeout_secs_param_accepts_string_integer() { + let timeout = serde_json::json!("30"); + assert_eq!(parse_timeout_secs_param(Some(&timeout)).unwrap(), Some(30)); + } + + #[test] + fn test_parse_timeout_secs_param_treats_empty_string_as_none() { + let timeout = serde_json::json!(""); + assert_eq!(parse_timeout_secs_param(Some(&timeout)).unwrap(), None); + } + + #[test] + fn test_parse_timeout_secs_param_rejects_value_above_cap() { + let timeout = serde_json::json!(MAX_TIMEOUT_SECS + 1); + let err = parse_timeout_secs_param(Some(&timeout)).unwrap_err(); + assert!( + err.to_string() + .contains(&format!("timeout_secs must be <= {}", MAX_TIMEOUT_SECS)), + "unexpected error: {}", + err + ); + } + + #[test] + fn test_parse_timeout_secs_param_rejects_string_value_above_cap() { + let timeout = serde_json::json!((MAX_TIMEOUT_SECS + 1).to_string()); + let err = parse_timeout_secs_param(Some(&timeout)).unwrap_err(); + assert!( + err.to_string() + .contains(&format!("timeout_secs must be <= {}", MAX_TIMEOUT_SECS)), + "unexpected error: {}", + err + ); + } + + #[test] + fn test_parse_save_to_param_treats_empty_string_as_none() { + let save_to = serde_json::json!(""); + assert_eq!(parse_save_to_param(Some(&save_to)).unwrap(), None); + } + #[test] fn test_http_tool_schema_body_is_freeform() { let schema = HttpTool::new().parameters_schema(); @@ -1119,6 +1275,25 @@ mod tests { assert_eq!(extract_host_from_params(¶ms), None); } + #[test] + fn test_requires_approval_with_stringified_http_params() { + use crate::tools::wasm::SharedCredentialRegistry; + + let tool = HttpTool::new().with_credentials( + Arc::new(SharedCredentialRegistry::new()), + Arc::new(test_secrets_store()), + ); + let req = serde_json::json!({ + "body": "", + "headers": "[]", + "method": "GET", + "save_to": "", + "timeout_secs": "30", + "url": "https://r.jina.ai/http://news.baidu.com/" + }); + let _ = tool.requires_approval(&req); + } + // ── DNS pinning tests ───────────────────────────────────────────── #[tokio::test]