Skip to content

feat: enhance HTTP tool parameter parsing #911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zmanian merged 5 commits into from
Mar 12, 2026
Merged

feat: enhance HTTP tool parameter parsing #911

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c01af22
feat: enhance HTTP tool parameter parsing
BenLocal Mar 11, 2026
b6e5a3a
Merge branch 'staging' into http_tool
BenLocal Mar 11, 2026
25445a4
feat(http): enhance HTTP tool with timeout and header parsing improve…
BenLocal Mar 11, 2026
703ec81
Merge branch 'staging' into http_tool
BenLocal Mar 12, 2026
0d43c78
refactor(http): replace hardcoded timeout with effective_timeout vari…
BenLocal Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
247 changes: 211 additions & 36 deletions src/tools/builtin/http.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -244,43 +250,120 @@ fn is_html_response(headers: &HashMap<String, String>) -> bool {
fn parse_headers_param(
headers: Option<&serde_json::Value>,
) -> Result<Vec<(String, String)>, ToolError> {
fn parse_header_object(
map: &serde_json::Map<String, serde_json::Value>,
) -> Result<Vec<(String, String)>, 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<Vec<(String, String)>, 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::<serde_json::Value>(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<Option<u64>, 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::<u64>().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<Option<String>, 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<String> {
params
Expand Down Expand Up @@ -358,6 +441,7 @@ impl Tool for HttpTool {
let start = std::time::Instant::now();

let method = require_str(&params, "method")?;
let method_upper = method.to_uppercase();

let url = require_str(&params, "url")?;
let mut parsed_url = validate_url(url)?;
Expand All @@ -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() {
Expand All @@ -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());
Expand All @@ -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::<serde_json::Value>(body_str) {
if body_str.is_empty() {
None
} else if let Ok(json_body) = serde_json::from_str::<serde_json::Value>(body_str) {
let bytes = serde_json::to_vec(&json_body).map_err(|e| {
ToolError::InvalidParameters(format!("invalid body JSON: {}", e))
})?;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
)?;

Expand All @@ -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())
}
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
})?;
Expand All @@ -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,
});
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -1119,6 +1275,25 @@ mod tests {
assert_eq!(extract_host_from_params(&params), 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]
Expand Down
Loading