From e805ec61aa6e744679cebb73b86bfc5e26ca5e6f Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Fri, 13 Mar 2026 09:04:55 -0700 Subject: [PATCH] fix: 5 critical/high-priority bugs (auth bypass, relay failures, unbounded recursion, context growth) (#1083) * fix: address 5 critical and high-priority bugs from issue tracker - #1033: reject webhook requests when secret is cleared at runtime via update_secret(None), preventing auth bypass through SIGHUP hot-swap - #908: reset consecutive_failures counter on successful SSE stream reconnection in relay channel, so circuit breaker counts truly consecutive failures - #975: add depth limit (16) to validate_tool_schema() to prevent stack overflow on deeply nested schemas - #974: add depth limit (8) to resolve_nested() to prevent stack overflow on deeply nested capabilities wrappers - #826: truncate oversized tool outputs (>8KB) in routine lightweight loop to prevent unbounded context growth across iterations Each fix includes a regression test. Closes #1033, #908, #975, #974, #826 Co-Authored-By: Claude Opus 4.6 * fix: 5 more high-priority bugs (routine cache, job signals, input limits) - #1077: recompute next_fire_at when re-enabling cron routines via web toggle, mirroring CLI behavior so cron ticker picks them up - #1076: refresh event trigger cache after web toggle/delete operations so event/system_event routines reflect changes immediately - #892: remove Stuck from check_signals() stop-states in JobDelegate since Stuck is recoverable (Stuck -> InProgress via self-repair) - #976: truncate oversized description strings in CapabilitiesFile to 4KB to prevent memory abuse from malicious capabilities files - #977: drop oversized parameters schema JSON (>64KB) in CapabilitiesFile to prevent unbounded memory growth Each fix includes regression tests where applicable. Closes #1077, #1076, #892, #976, #977 Co-Authored-By: Claude Opus 4.6 * fix: prevent ReDoS in event trigger regex patterns - #825: use RegexBuilder with 64KB size limit when compiling user-supplied event trigger patterns, both at creation time (routine tool) and at cache refresh (routine engine) Note: Rust's regex crate already guarantees O(n) matching, so the size limit prevents excessive memory use during compilation rather than catastrophic backtracking at match time. Closes #825 Co-Authored-By: Claude Opus 4.6 * Harden HTTP SSRF IP filtering * Apply rustfmt after staging merge --------- Co-authored-by: Claude Opus 4.6 --- src/agent/routine_engine.rs | 43 +++++++--- src/channels/http.rs | 50 +++++++++-- src/channels/relay/channel.rs | 4 + src/channels/web/handlers/routines.rs | 27 +++++- src/tools/builtin/http.rs | 3 + src/tools/builtin/routine.rs | 10 ++- src/tools/tool.rs | 51 ++++++++++- src/tools/wasm/capabilities_schema.rs | 118 +++++++++++++++++++++++++- src/worker/job.rs | 8 +- 9 files changed, 277 insertions(+), 37 deletions(-) diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index b4aa5e0c..a34654e9 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -93,19 +93,26 @@ impl RoutineEngine { let mut cache = Vec::new(); for routine in routines { match &routine.trigger { - Trigger::Event { pattern, .. } => match Regex::new(pattern) { - Ok(re) => cache.push(EventMatcher::Message { - routine: routine.clone(), - regex: re, - }), - Err(e) => { - tracing::warn!( - routine = %routine.name, - "Invalid event regex '{}': {}", - pattern, e - ); + Trigger::Event { pattern, .. } => { + // Use RegexBuilder with size limit to prevent ReDoS + // from user-supplied patterns (issue #825). + match regex::RegexBuilder::new(pattern) + .size_limit(64 * 1024) // 64KB compiled size limit + .build() + { + Ok(re) => cache.push(EventMatcher::Message { + routine: routine.clone(), + regex: re, + }), + Err(e) => { + tracing::warn!( + routine = %routine.name, + "Invalid or too complex event regex '{}': {}", + pattern, e + ); + } } - }, + } Trigger::SystemEvent { .. } => { cache.push(EventMatcher::System { routine: routine.clone(), @@ -973,6 +980,18 @@ async fn execute_lightweight_with_tools( } }; + // Truncate oversized tool output to prevent unbounded context growth. + // Routine tool loops are lightweight and should not accumulate + // large payloads across iterations. + const MAX_TOOL_OUTPUT_CHARS: usize = 8192; + let result_content = if result_content.len() > MAX_TOOL_OUTPUT_CHARS { + let truncated = &result_content + [..result_content.floor_char_boundary(MAX_TOOL_OUTPUT_CHARS)]; + format!("{truncated}\n... [output truncated to {MAX_TOOL_OUTPUT_CHARS} chars]") + } else { + result_content + }; + // Add tool result to context messages.push(ChatMessage::tool_result(&tc.id, &tc.name, &result_content)); } diff --git a/src/channels/http.rs b/src/channels/http.rs index 15468c6a..00a48048 100644 --- a/src/channels/http.rs +++ b/src/channels/http.rs @@ -269,21 +269,25 @@ async fn webhook_handler( let mut fallback_req = None; { let webhook_secret = state.webhook_secret.read().await; - let Some(expected_secret) = webhook_secret.as_ref() else { + if webhook_secret.is_none() { + // No secret configured — reject all requests. This guards against + // the secret being cleared at runtime via update_secret(None). + // The start() method also prevents startup without a secret, but + // this is defense-in-depth for the SIGHUP hot-swap path. return ( - StatusCode::UNAUTHORIZED, + StatusCode::SERVICE_UNAVAILABLE, Json(WebhookResponse { message_id: Uuid::nil(), status: "error".to_string(), - response: Some( - "Webhook authentication required: HTTP webhook secret is not configured." - .to_string(), - ), + response: Some("Webhook authentication not configured".to_string()), }), ) .into_response(); - }; - let expected_secret = expected_secret.expose_secret(); + } + let expected_secret = webhook_secret + .as_ref() + .expect("checked is_none above") + .expose_secret(); match headers.get("x-ironclaw-signature") { Some(raw_signature) => match raw_signature.to_str() { @@ -1206,4 +1210,34 @@ mod tests { let body = b"test body content"; assert!(!verify_hmac_signature(secret, body, "sha256=not-hex!")); } + + /// Regression test for issue #1033: when the webhook secret is cleared at + /// runtime via update_secret(None), subsequent requests must be rejected + /// instead of being processed without authentication. + #[tokio::test] + async fn webhook_rejects_when_secret_cleared_at_runtime() { + let channel = test_channel(Some("initial-secret")); + let _stream = channel.start().await.unwrap(); + + // Clear the secret at runtime (simulates a bad SIGHUP config reload) + channel.update_secret(None).await; + + let app = channel.routes(); + let body = serde_json::json!({ + "content": "hello" + }); + let req = Request::builder() + .method("POST") + .uri("/webhook") + .header("content-type", "application/json") + .body(Body::from(serde_json::to_vec(&body).unwrap())) + .unwrap(); + + let resp = app.oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "requests must be rejected when webhook secret is cleared at runtime" + ); + } } diff --git a/src/channels/relay/channel.rs b/src/channels/relay/channel.rs index d6aa90cc..52aea478 100644 --- a/src/channels/relay/channel.rs +++ b/src/channels/relay/channel.rs @@ -294,6 +294,8 @@ impl Channel for RelayChannel { match client.connect_stream(&token, stream_timeout_secs).await { Ok((new_stream, new_parser)) => { tracing::info!("Relay SSE stream reconnected"); + consecutive_failures = 0; + backoff_ms = backoff_initial_ms; current_stream = new_stream; // Abort old parser before replacing if let Some(old) = parser_handle.write().await.take() { @@ -312,6 +314,8 @@ impl Channel for RelayChannel { tracing::info!( "Relay SSE stream reconnected with new token" ); + consecutive_failures = 0; + backoff_ms = backoff_initial_ms; current_stream = new_stream; if let Some(old) = parser_handle.write().await.take() { old.abort(); diff --git a/src/channels/web/handlers/routines.rs b/src/channels/web/handlers/routines.rs index f49d7fe8..f5d8db02 100644 --- a/src/channels/web/handlers/routines.rs +++ b/src/channels/web/handlers/routines.rs @@ -190,12 +190,21 @@ pub async fn routines_toggle_handler( None => !routine.enabled, }; + // When re-enabling a cron routine, recompute next_fire_at so the cron + // ticker can pick it up. Mirrors the CLI behavior (issue #1077). if routine.enabled && !was_enabled - && let Trigger::Cron { schedule, timezone } = &routine.trigger + && let Trigger::Cron { + ref schedule, + ref timezone, + } = routine.trigger { - routine.next_fire_at = next_cron_fire(schedule, timezone.as_deref()) - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + routine.next_fire_at = next_cron_fire(schedule, timezone.as_deref()).map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + format!("Failed to compute next fire: {e}"), + ) + })?; } store @@ -203,6 +212,12 @@ pub async fn routines_toggle_handler( .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + // Refresh the in-memory event trigger cache so event/system_event + // routines reflect the new enabled state immediately (issue #1076). + if let Some(engine) = state.routine_engine.read().await.as_ref() { + engine.refresh_event_cache().await; + } + Ok(Json(serde_json::json!({ "status": if routine.enabled { "enabled" } else { "disabled" }, "routine_id": routine_id, @@ -227,6 +242,12 @@ pub async fn routines_delete_handler( .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; if deleted { + // Refresh the in-memory event trigger cache so deleted event/system_event + // routines stop firing immediately (issue #1076). + if let Some(engine) = state.routine_engine.read().await.as_ref() { + engine.refresh_event_cache().await; + } + Ok(Json(serde_json::json!({ "status": "deleted", "routine_id": routine_id, diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index 4ed2bb0b..9d7af888 100644 --- a/src/tools/builtin/http.rs +++ b/src/tools/builtin/http.rs @@ -214,6 +214,7 @@ fn is_disallowed_ipv4(v4: &Ipv4Addr) -> bool { || v4.is_multicast() || v4.is_unspecified() || *v4 == Ipv4Addr::new(169, 254, 169, 254) + || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) } fn is_disallowed_ip(ip: &IpAddr) -> bool { @@ -913,6 +914,8 @@ mod tests { assert!(is_disallowed_ip(&IpAddr::V4(Ipv4Addr::new( 169, 254, 169, 254 )))); + // Carrier-grade NAT + assert!(is_disallowed_ip(&IpAddr::V4(Ipv4Addr::new(100, 64, 0, 1)))); // Public assert!(!is_disallowed_ip(&IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)))); } diff --git a/src/tools/builtin/routine.rs b/src/tools/builtin/routine.rs index 43e2add7..577cd2d1 100644 --- a/src/tools/builtin/routine.rs +++ b/src/tools/builtin/routine.rs @@ -199,9 +199,13 @@ impl Tool for RoutineCreateTool { "event trigger requires 'event_pattern'".to_string(), ) })?; - // Validate regex - regex::Regex::new(pattern) - .map_err(|e| ToolError::InvalidParameters(format!("invalid regex: {e}")))?; + // Validate regex with size limit to prevent ReDoS (issue #825) + regex::RegexBuilder::new(pattern) + .size_limit(64 * 1024) + .build() + .map_err(|e| { + ToolError::InvalidParameters(format!("invalid or too complex regex: {e}")) + })?; let channel = params .get("event_channel") .and_then(|v| v.as_str()) diff --git a/src/tools/tool.rs b/src/tools/tool.rs index 4a0fda8d..608c71a6 100644 --- a/src/tools/tool.rs +++ b/src/tools/tool.rs @@ -430,9 +430,24 @@ pub fn redact_params(params: &serde_json::Value, sensitive: &[&str]) -> serde_js /// Properties without a `"type"` field are allowed (freeform/any-type). /// This is an intentional pattern used by tools like `json` and `http` for /// OpenAI compatibility, since union types with arrays require `items`. +/// Maximum nesting depth for tool schema validation to prevent stack overflow +/// on maliciously crafted schemas. +const MAX_SCHEMA_DEPTH: usize = 16; + pub fn validate_tool_schema(schema: &serde_json::Value, path: &str) -> Vec { + validate_tool_schema_inner(schema, path, 0) +} + +fn validate_tool_schema_inner(schema: &serde_json::Value, path: &str, depth: usize) -> Vec { let mut errors = Vec::new(); + if depth > MAX_SCHEMA_DEPTH { + errors.push(format!( + "{path}: schema nesting exceeds maximum depth of {MAX_SCHEMA_DEPTH}" + )); + return errors; + } + // Rule 1: must have "type": "object" at this level match schema.get("type").and_then(|t| t.as_str()) { Some("object") => {} @@ -474,14 +489,17 @@ pub fn validate_tool_schema(schema: &serde_json::Value, path: &str) -> Vec { - errors.extend(validate_tool_schema(prop, &prop_path)); + errors.extend(validate_tool_schema_inner(prop, &prop_path, depth + 1)); } "array" => { if let Some(items) = prop.get("items") { // If items is an object type, recurse if items.get("type").and_then(|t| t.as_str()) == Some("object") { - errors - .extend(validate_tool_schema(items, &format!("{prop_path}.items"))); + errors.extend(validate_tool_schema_inner( + items, + &format!("{prop_path}.items"), + depth + 1, + )); } } else { errors.push(format!("{prop_path}: array property missing \"items\"")); @@ -810,6 +828,33 @@ mod tests { assert!(errors[0].contains("\"missing_field\"")); } + /// Regression test for issue #975: deeply nested schemas must not cause + /// stack overflow. The validator should stop at MAX_SCHEMA_DEPTH and + /// report an error instead of recursing infinitely. + #[test] + fn test_validate_schema_depth_limit() { + // Build a schema nested 20 levels deep (exceeds MAX_SCHEMA_DEPTH=16) + let mut schema = serde_json::json!({ + "type": "object", + "properties": { + "leaf": { "type": "string" } + } + }); + for _ in 0..20 { + schema = serde_json::json!({ + "type": "object", + "properties": { + "nested": schema + } + }); + } + let errors = validate_tool_schema(&schema, "test"); + assert!( + errors.iter().any(|e| e.contains("maximum depth")), + "expected depth limit error, got: {errors:?}" + ); + } + #[test] fn test_approval_context_autonomous_allows_unless_auto_approved() { let ctx = ApprovalContext::autonomous(); diff --git a/src/tools/wasm/capabilities_schema.rs b/src/tools/wasm/capabilities_schema.rs index 60cae7c2..1c1685ee 100644 --- a/src/tools/wasm/capabilities_schema.rs +++ b/src/tools/wasm/capabilities_schema.rs @@ -101,24 +101,75 @@ pub struct CapabilitiesFile { pub capabilities: Option>, } +/// Maximum length for the description field to prevent memory abuse. +const MAX_DESCRIPTION_CHARS: usize = 4096; +/// Maximum serialized size of the parameters schema JSON. +const MAX_PARAMETERS_SCHEMA_BYTES: usize = 64 * 1024; + impl CapabilitiesFile { /// Parse from JSON string. pub fn from_json(json: &str) -> Result { - serde_json::from_str::(json).map(Self::resolve_nested) + let mut caps = serde_json::from_str::(json).map(Self::resolve_nested)?; + caps.enforce_limits(); + Ok(caps) } /// Parse from JSON bytes. pub fn from_bytes(bytes: &[u8]) -> Result { - serde_json::from_slice::(bytes).map(Self::resolve_nested) + let mut caps = serde_json::from_slice::(bytes).map(Self::resolve_nested)?; + caps.enforce_limits(); + Ok(caps) + } + + /// Truncate oversized fields to prevent unbounded memory usage. + fn enforce_limits(&mut self) { + // Truncate oversized description (issue #976) + if let Some(ref desc) = self.description + && desc.len() > MAX_DESCRIPTION_CHARS + { + let truncated = &desc[..desc.floor_char_boundary(MAX_DESCRIPTION_CHARS)]; + tracing::warn!( + "Capabilities description truncated from {} to {} chars", + desc.len(), + MAX_DESCRIPTION_CHARS, + ); + self.description = Some(truncated.to_string()); + } + // Drop oversized parameters schema (issue #977) + if let Some(ref params) = self.parameters { + let size = params.to_string().len(); + if size > MAX_PARAMETERS_SCHEMA_BYTES { + tracing::warn!( + "Capabilities parameters schema dropped ({} bytes exceeds {} limit)", + size, + MAX_PARAMETERS_SCHEMA_BYTES, + ); + self.parameters = None; + } + } } /// Merge nested `capabilities` wrapper into top-level fields. /// /// Channel-level JSON nests tool capabilities under `"capabilities"`. /// This promotes the inner fields so callers can access them uniformly. - fn resolve_nested(mut self) -> Self { + /// Maximum nesting depth for capabilities resolution. + const MAX_NESTED_DEPTH: usize = 8; + + fn resolve_nested(self) -> Self { + self.resolve_nested_inner(0) + } + + fn resolve_nested_inner(mut self, depth: usize) -> Self { + if depth > Self::MAX_NESTED_DEPTH { + tracing::warn!( + "Capabilities nesting exceeds maximum depth of {}, stopping resolution", + Self::MAX_NESTED_DEPTH + ); + return self; + } if let Some(inner) = self.capabilities.take() { - let inner = inner.resolve_nested(); + let inner = inner.resolve_nested_inner(depth + 1); self.description = self.description.or(inner.description); self.parameters = self.parameters.or(inner.parameters); self.http = self.http.or(inner.http); @@ -1383,4 +1434,63 @@ mod tests { "Outer description should take precedence over inner" ); } + + /// Regression test for issue #974: deeply nested capabilities wrappers + /// must not cause stack overflow. resolve_nested should stop at + /// MAX_NESTED_DEPTH and return gracefully. + #[test] + fn test_resolve_nested_depth_limit() { + // Build a capabilities file nested beyond MAX_NESTED_DEPTH (8). + // The description is at the innermost level which is beyond the limit, + // so it won't be resolved — the key assertion is no stack overflow. + let mut json = r#"{ "description": "leaf" }"#.to_string(); + for _ in 0..20 { + json = format!(r#"{{ "capabilities": {json} }}"#); + } + // Should not stack overflow — this is the primary assertion. + let _caps = CapabilitiesFile::from_json(&json).unwrap(); + } + + /// Regression test for issue #976: oversized description strings are truncated. + #[test] + fn test_description_truncated_at_limit() { + let long_desc = "x".repeat(10_000); + let json = format!(r#"{{ "description": "{long_desc}" }}"#); + let caps = CapabilitiesFile::from_json(&json).unwrap(); + let desc = caps.description.unwrap(); + assert!( + desc.len() <= super::MAX_DESCRIPTION_CHARS + 50, // allow for minor overhead + "description should be truncated to ~{} chars, got {}", + super::MAX_DESCRIPTION_CHARS, + desc.len() + ); + } + + /// Regression test for issue #977: oversized parameters schema is dropped. + #[test] + fn test_oversized_parameters_schema_dropped() { + // Build a parameters schema larger than MAX_PARAMETERS_SCHEMA_BYTES + let mut properties = serde_json::Map::new(); + for i in 0..2000 { + properties.insert( + format!("field_{i}"), + serde_json::json!({ + "type": "string", + "description": "x".repeat(50) + }), + ); + } + let schema = serde_json::json!({ + "type": "object", + "properties": properties, + }); + let json = serde_json::json!({ + "parameters": schema, + }); + let caps = CapabilitiesFile::from_json(&json.to_string()).unwrap(); + assert!( + caps.parameters.is_none(), + "oversized parameters schema should be dropped" + ); + } } diff --git a/src/worker/job.rs b/src/worker/job.rs index 1f207435..86363f38 100644 --- a/src/worker/job.rs +++ b/src/worker/job.rs @@ -1108,9 +1108,10 @@ impl<'a> LoopDelegate for JobDelegate<'a> { return LoopSignal::InjectMessage(content); } - // Check for terminal or non-progressing state. The loop should stop when the - // job has been cancelled, failed, stuck, or already completed — not just the - // three states that `is_terminal()` covers (Accepted/Failed/Cancelled). + // Check for terminal or post-completion state. The loop should stop when the + // job has been cancelled, failed, or already completed — but NOT when Stuck, + // because Stuck is recoverable (Stuck -> InProgress via self-repair). + // Stopping on Stuck would prevent recovery from resuming the worker (issue #892). if let Ok(ctx) = self .worker .context_manager() @@ -1120,7 +1121,6 @@ impl<'a> LoopDelegate for JobDelegate<'a> { ctx.state, JobState::Cancelled | JobState::Failed - | JobState::Stuck | JobState::Completed | JobState::Submitted | JobState::Accepted