From 70c12ad2abfdff9ddc72e82daa3e3f6e6ba58b90 Mon Sep 17 00:00:00 2001 From: Cyrus Gray <144336577+graycyrus@users.noreply.github.com> Date: Fri, 17 Jul 2026 00:10:39 +0530 Subject: [PATCH] fix(flows): close Composio tool_call tier-gate bypass (C1) (#5000) --- src/openhuman/flows/ops.rs | 65 +++++++++++++++++++-- src/openhuman/flows/ops_tests.rs | 71 +++++++++++++++++++++++ src/openhuman/tinyflows/caps.rs | 89 +++++++++++++++++++++-------- src/openhuman/tinyflows/tests.rs | 7 ++- tests/json_rpc_e2e.rs | 97 ++++++++++++++++++++++++++++++++ 5 files changed, 300 insertions(+), 29 deletions(-) diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 2075d2072..1ee18d056 100644 --- a/src/openhuman/flows/ops.rs +++ b/src/openhuman/flows/ops.rs @@ -366,6 +366,28 @@ pub(crate) fn graph_has_outbound_side_effect(graph: &WorkflowGraph) -> bool { }) } +/// Shared Rule 2 enforcement (issue B29, and its `flows_update` compound-bypass +/// closure): forces `require_approval` to `true` when `graph` contains an +/// outbound side-effect node, no matter what the caller asked for. Used by both +/// [`flows_create`] and [`flows_update`] so a flow can never persist +/// `require_approval: false` alongside a `tool_call` / `http_request` / `code` +/// node — on create OR on a later edit that *adds* such a node to a +/// previously-read-only graph. +/// +/// Returns `(effective_require_approval, was_forced)`: `was_forced` is `true` +/// only when the caller's own toggle was `false` but a side-effect node +/// required the override — callers use it to decide whether to emit the +/// loud "forced to true" log/result note. +pub(crate) fn enforce_side_effect_approval( + graph: &WorkflowGraph, + caller_require_approval: bool, +) -> (bool, bool) { + let has_side_effect = graph_has_outbound_side_effect(graph); + let effective_require_approval = caller_require_approval || has_side_effect; + let was_forced = has_side_effect && !caller_require_approval; + (effective_require_approval, was_forced) +} + /// Whether `graph` has anything for [`flows_run`] to actually *do* — i.e. at /// least one non-`trigger` node **reachable from the trigger** by following /// directed edges. A graph made of nothing but a bare `trigger` node (or a @@ -2178,9 +2200,9 @@ pub async fn flows_create( // Rule 2: any outbound side-effect node forces require_approval, no // matter what the caller asked for. - let has_side_effect = graph_has_outbound_side_effect(&graph); - let effective_require_approval = require_approval || has_side_effect; - if has_side_effect && !require_approval { + let (effective_require_approval, side_effect_forced) = + enforce_side_effect_approval(&graph, require_approval); + if side_effect_forced { tracing::info!( target: "flows", %name, @@ -2218,7 +2240,7 @@ pub async fn flows_create( Enable it explicitly (flows_set_enabled) when you are ready for it to fire." )); } - if effective_require_approval && !require_approval { + if side_effect_forced { logs.push( "require_approval forced to true because the graph contains outbound side-effect \ nodes (tool_call / http_request / code)." @@ -2638,7 +2660,31 @@ pub async fn flows_update( "[flows] flows_update: auto-trigger disarm decision inputs" ); - tracing::debug!(target: "flows", flow_id = %id, has_expected = expected_version.is_some(), "[flows] flows_update: persisting changes"); + // Rule 2 analogue (compound-bypass closure): re-apply the same outbound + // side-effect check `flows_create` applies on save — via the shared + // [`enforce_side_effect_approval`] helper — so an update that *adds* a + // tool_call/http_request/code node to a previously read-only graph can + // never persist `require_approval: false` just because the update path + // trusted the caller's toggle unconditionally. + let (effective_require_approval, side_effect_forced) = + enforce_side_effect_approval(&graph, new_require_approval); + if side_effect_forced { + tracing::info!( + target: "flows", + flow_id = %id, + "[flows] flows_update: forcing require_approval=true — graph contains outbound \ + side-effect node(s) (tool_call / http_request / code)" + ); + } + + tracing::debug!( + target: "flows", + flow_id = %id, + has_expected = expected_version.is_some(), + require_approval = effective_require_approval, + side_effect_forced, + "[flows] flows_update: persisting changes" + ); // `enabled_override` is threaded into the same guarded UPDATE as the // graph/name/require_approval write (see `store::update_flow_graph`) // rather than a follow-up `flows_set_enabled` call, so the disarm can @@ -2648,7 +2694,7 @@ pub async fn flows_update( id, new_name, graph, - new_require_approval, + effective_require_approval, enabled_override, expected_version.as_deref(), ) @@ -2683,6 +2729,13 @@ pub async fn flows_update( .to_string(), ); } + if side_effect_forced { + logs.push( + "require_approval forced to true because the graph contains outbound side-effect \ + nodes (tool_call / http_request / code)." + .to_string(), + ); + } Ok(RpcOutcome::new(updated, logs)) } diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 09a9e9db5..2cd76a708 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/src/openhuman/flows/ops_tests.rs @@ -4337,6 +4337,77 @@ async fn flows_create_schedule_outbound_creates_disabled_and_approval() { ); } +#[tokio::test] +async fn flows_update_forces_require_approval_when_adding_side_effect_nodes() { + // Compound bypass fix, half 2: `flows_create`'s Rule 2 (force + // require_approval when the graph gains an outbound side-effect node) + // must also re-apply on `flows_update` — a flow that starts read-only and + // is later edited to add a Composio/http_request/code node must not be + // able to keep require_approval=false just because the update path never + // re-checked. + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + let created = flows_create(&config, "demo".to_string(), trigger_only_graph(), false) + .await + .unwrap(); + assert!( + !created.value.require_approval, + "a trigger-only graph must not force require_approval on create" + ); + + let updated = flows_update( + &config, + &created.value.id, + None, + Some(tool_call_graph()), + Some(false), + None, + ) + .await + .unwrap(); + + assert!( + updated.value.require_approval, + "flows_update must force require_approval when the replacement graph adds an outbound \ + side-effect node (tool_call), even though the caller passed false" + ); + assert!( + updated + .logs + .iter() + .any(|l| l.contains("require_approval forced to true")), + "flows_update must loudly log the forced require_approval: {:?}", + updated.logs + ); +} + +#[tokio::test] +async fn flows_update_does_not_force_require_approval_on_readonly_graph() { + let tmp = TempDir::new().unwrap(); + let config = test_config(&tmp); + let created = flows_create(&config, "demo".to_string(), trigger_only_graph(), false) + .await + .unwrap(); + assert!(!created.value.require_approval); + + // Name-only update — no graph change, no side-effect nodes. + let updated = flows_update( + &config, + &created.value.id, + Some("renamed".to_string()), + None, + None, + None, + ) + .await + .unwrap(); + + assert!( + !updated.value.require_approval, + "a name-only update to a read-only graph must not force require_approval" + ); +} + // ── graph_has_outbound_side_effect / trigger_is_automatic helper tests ──── #[test] diff --git a/src/openhuman/tinyflows/caps.rs b/src/openhuman/tinyflows/caps.rs index 6e2470b06..2d38d66bd 100644 --- a/src/openhuman/tinyflows/caps.rs +++ b/src/openhuman/tinyflows/caps.rs @@ -1616,6 +1616,7 @@ async fn resolve_composio_account( /// // approval when a trigger-driven run's tool/http config contains `=`-exprs. pub struct OpenHumanTools { pub config: Arc, + pub security: Arc, } /// Prefix marking a `tool_call` node's slug as a NATIVE OpenHuman tool (the @@ -2562,18 +2563,13 @@ impl ToolInvoker for OpenHumanTools { )); } - let security = SecurityPolicy::from_config( - &self.config.autonomy, - &self.config.workspace_dir, - &self.config.action_dir, - ); let class = crate::openhuman::runtime_node::ops::classify_tool_call( &self.config, tool_name, &args, ) .map_err(EngineError::Capability)?; - let tier_decision = enforce_node_tier_gate(&security, class, "tool_call")?; + let tier_decision = enforce_node_tier_gate(&self.security, class, "tool_call")?; let summary = crate::openhuman::approval::summarize_action(tool_name, &args); let redacted = crate::openhuman::approval::redact_args(&args); let (outcome, _request_id) = @@ -2601,6 +2597,23 @@ impl ToolInvoker for OpenHumanTools { }); } + // Autonomy-tier gate (Phase 2): a Composio action reaches a + // third-party API over the network, so it is Network-class — same + // class as `http_request`. A read-only run `Block`s here and never + // reaches curation, the preflight, or the approval gate; + // Supervised/Full fall through to `gate_call_for_tier` below. Runs + // BEFORE the curation check (unlike the pre-fix behavior) so a + // read-only tier can never even probe which slugs are curated. + let tier_decision = + enforce_node_tier_gate(&self.security, CommandClass::Network, "tool_call")?; + tracing::debug!( + target: "flows", + %slug, + ?tier_decision, + tier = ?self.security.autonomy, + "[flows] tool_call: composio node tier gate evaluated" + ); + // Curation + scope gate — hard allowlist (see [`is_curated_flow_tool`]'s // doc for why this differs from the general agent tool-call path). // Runs before anything else — a rejected slug never reaches the @@ -2623,22 +2636,24 @@ impl ToolInvoker for OpenHumanTools { // provider or asks the user to approve a call that cannot succeed. preflight_composio_args(&self.config, slug, &args).await?; - // Approval gate (see the struct doc). Mirrors - // `tinyagents/middleware.rs::ApprovalSecurityMiddleware::wrap_tool`'s - // shape exactly: compute summary/redacted args only when a gate is - // installed, deny short-circuits before any composio call, allow - // records an audit id to close out after the call resolves. - let mut audit_id: Option = None; - if let Some(gate) = crate::openhuman::approval::ApprovalGate::try_global() { - let summary = crate::openhuman::approval::summarize_action(slug, &args); - let redacted = crate::openhuman::approval::redact_args(&args); - let (outcome, request_id) = gate.intercept_audited(slug, &summary, redacted).await; - match outcome { - crate::openhuman::approval::GateOutcome::Deny { reason } => { - return Err(EngineError::Capability(reason)); - } - crate::openhuman::approval::GateOutcome::Allow => audit_id = request_id, - } + // Approval gate (see the struct doc). Mirrors `OpenHumanHttp::request`'s + // shape exactly: `gate_call_for_tier` is what actually performs the + // `Prompt` round-trip — it escalates a Supervised `Prompt` decision + // into a forced approval regardless of the flow's own + // `require_approval` toggle (Codex P1), same as the `http_request` and + // `code` node paths. Deny short-circuits before any composio call; + // Allow records an audit id to close out after the call resolves. + let summary = crate::openhuman::approval::summarize_action(slug, &args); + let redacted = crate::openhuman::approval::redact_args(&args); + let (outcome, audit_id) = gate_call_for_tier(tier_decision, slug, &summary, redacted).await; + if let crate::openhuman::approval::GateOutcome::Deny { reason } = outcome { + tracing::warn!( + target: "flows", + %slug, + ?tier_decision, + "[flows] tool_call: approval gate denied before Composio dispatch" + ); + return Err(EngineError::Capability(reason)); } let kind = create_composio_client(&self.config) @@ -3277,6 +3292,7 @@ pub fn build_capabilities(config: Arc, state_namespace: impl Into) -> OpenHumanTools { - OpenHumanTools { config } + let security = Arc::new(SecurityPolicy::from_config( + &config.autonomy, + &config.workspace_dir, + &config.action_dir, + )); + OpenHumanTools { config, security } } #[tokio::test] diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index e374fa918..a1463bb2e 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -12862,6 +12862,103 @@ async fn json_rpc_flows_lifecycle_round_trip() { rpc_join.abort(); } +/// JSON-RPC regression for the Rule 2 compound-bypass fix (C1): `flows_update` +/// must re-derive `require_approval` from the *effective* graph, not trust the +/// caller's raw toggle. This is the RPC-layer counterpart to the direct-API +/// tests `flows_update_forces_require_approval_when_adding_side_effect_nodes` +/// / `flows_update_does_not_force_require_approval_on_readonly_graph` in +/// `src/openhuman/flows/ops_tests.rs` — same rule, exercised through the +/// `openhuman.flows_update` controller (schema + handler wiring), not just +/// the `ops::flows_update` fn directly. +#[cfg(feature = "flows")] +#[tokio::test] +async fn json_rpc_flows_update_forces_require_approval_on_side_effect_graph() { + let _env_lock = json_rpc_e2e_env_lock(); + let (rpc_base, _tmp, api_join, rpc_join, _guards) = boot_flows_rpc_env().await; + + // 1. Create a trigger-only (read-only) flow with require_approval: false. + let create = post_json_rpc( + &rpc_base, + 9401, + "openhuman.flows_create", + json!({ + "name": "rpc-rule2-demo", + "graph": { + "name": "trigger-only", + "nodes": [ { "id": "t", "kind": "trigger", "name": "Trigger" } ], + "edges": [] + }, + "require_approval": false + }), + ) + .await; + let flow = peel_logs_envelope(assert_no_jsonrpc_error(&create, "flows_create")); + let flow_id = flow + .get("id") + .and_then(Value::as_str) + .expect("flow id in create result") + .to_string(); + assert_eq!( + flow.get("require_approval").and_then(Value::as_bool), + Some(false), + "a trigger-only graph must not force require_approval on create" + ); + + // 2. Update to a graph with an outbound Composio tool_call node, still + // passing require_approval: false — the RPC handler must force it to + // true rather than trust the caller's toggle. + let update = post_json_rpc( + &rpc_base, + 9402, + "openhuman.flows_update", + json!({ + "id": flow_id, + "graph": { + "name": "with-tool-call", + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Trigger" }, + { + "id": "post", + "kind": "tool_call", + "name": "Post", + "config": { "slug": "SLACK_SEND_MESSAGE", "args": { "channel": "general" } } + } + ], + "edges": [ { "from_node": "t", "to_node": "post" } ] + }, + "require_approval": false + }), + ) + .await; + let updated = peel_logs_envelope(assert_no_jsonrpc_error(&update, "flows_update")); + assert_eq!( + updated.get("require_approval").and_then(Value::as_bool), + Some(true), + "flows_update over JSON-RPC must force require_approval=true when the \ + replacement graph adds an outbound side-effect node, even though the \ + caller passed false" + ); + + // 3. The forced value must also be what's persisted, not just what's + // echoed back in the update response. + let get = post_json_rpc( + &rpc_base, + 9403, + "openhuman.flows_get", + json!({ "id": flow_id }), + ) + .await; + let persisted = peel_logs_envelope(assert_no_jsonrpc_error(&get, "flows_get")); + assert_eq!( + persisted.get("require_approval").and_then(Value::as_bool), + Some(true), + "the forced require_approval must be persisted, not just returned" + ); + + api_join.abort(); + rpc_join.abort(); +} + /// Flow Scout suggestion-lifecycle methods over JSON-RPC (no LLM involved): /// `flows_list_suggestions` starts empty, and `flows_dismiss_suggestion` / /// `flows_mark_suggestion_built` on an unknown id resolve cleanly and report