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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Harden HTTP SSRF IP filtering * Apply rustfmt after staging merge --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
2026-08-25 14:53:34 +00:00 · 2026-03-13 16:04:55 +00:00
parent 1e00b1fed5
commit e805ec61aa
9 changed files with 277 additions and 37 deletions
@@ -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));
            }
@@ -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"
+        );
+    }
 }
@@ -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();
@@ -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,
@@ -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))));
    }
@@ -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())
@@ -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<String> {
+    validate_tool_schema_inner(schema, path, 0)
+}
+
+fn validate_tool_schema_inner(schema: &serde_json::Value, path: &str, depth: usize) -> Vec<String> {
    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<Strin
        if let Some(prop_type) = prop.get("type").and_then(|t| t.as_str()) {
            match prop_type {
                "object" => {
-                    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();
@@ -101,24 +101,75 @@ pub struct CapabilitiesFile {
    pub capabilities: Option<Box<CapabilitiesFile>>,
 }

+/// 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<Self, serde_json::Error> {
-        serde_json::from_str::<Self>(json).map(Self::resolve_nested)
+        let mut caps = serde_json::from_str::<Self>(json).map(Self::resolve_nested)?;
+        caps.enforce_limits();
+        Ok(caps)
    }

    /// Parse from JSON bytes.
    pub fn from_bytes(bytes: &[u8]) -> Result<Self, serde_json::Error> {
-        serde_json::from_slice::<Self>(bytes).map(Self::resolve_nested)
+        let mut caps = serde_json::from_slice::<Self>(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"
+        );
+    }
 }
@@ -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