Repository navigation
fix(flows): close Composio tool_call tier-gate bypass (C1) - #5000
Conversation
Two compounding gaps let a flow gain an unattended, unapproved outbound Composio call even under a read-only autonomy tier: 1. The Composio branch of OpenHumanTools::invoke (caps.rs) called intercept_audited directly, skipping the enforce_node_tier_gate + gate_call_for_tier pair that the native `oh:`, http_request, and code node paths all go through. OpenHumanTools now carries a SecurityPolicy and gates Composio dispatch as Network-class, before curation, mirroring OpenHumanHttp::request exactly. 2. flows_update never re-applied flows_create's Rule 2 (force require_approval when the graph contains an outbound side-effect node), so editing a saved read-only flow to add a tool_call/http_request/code node could keep require_approval=false. Both flows_create and flows_update now go through one shared enforce_side_effect_approval helper in ops.rs. Failing-test-first: composio_tool_call_blocks_under_readonly_tier (caps.rs) and flows_update_forces_require_approval_when_adding_side_effect_nodes (ops_tests.rs) were added and confirmed failing before the fix, plus a flows_update_does_not_force_require_approval_on_readonly_graph guard test. Platform-agnostic — SLACK_SEND_MESSAGE is only a sample slug.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFlow creation and updates now share outbound side-effect approval enforcement. Tool invocation receives an injected security policy, gates Composio calls earlier by autonomy tier, and uses tier-aware approval handling with regression coverage. ChangesApproval and security enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/flows/ops.rs (1)
2669-2670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the effective approval decision in the update persistence log.
Lines 2669-2670 compute the security-sensitive effective value, but the adjacent
flows_update: persisting changesevent records onlyhas_expected. Addrequire_approval = effective_require_approvalandside_effect_forcedso both branches are grep-friendly and auditable.As per coding guidelines, new or changed flows must include verbose, grep-friendly diagnostics covering branches and state transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/flows/ops.rs` around lines 2669 - 2670, Update the adjacent “flows_update: persisting changes” event to include both effective_require_approval and side_effect_forced alongside has_expected. Use the values returned by enforce_side_effect_approval, preserving the existing branch/state-transition diagnostics and making both approval outcomes explicitly auditable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 4340-4382: The existing direct API test in
flows_update_forces_require_approval_when_adding_side_effect_nodes lacks
JSON-RPC coverage. Add a JSON-RPC regression test that updates a flow with
require_approval set to false and a side-effect graph, then assert the RPC
response and persisted flow both report require_approval as true.
In `@src/openhuman/tinyflows/caps.rs`:
- Around line 2639-2650: In the approval-gate branch within the flow path around
gate_call_for_tier, log a flow-level diagnostic immediately before returning a
GateOutcome::Deny error. Include only the action slug and tier decision,
excluding reason and args, then preserve the existing EngineError::Capability
return behavior.
---
Nitpick comments:
In `@src/openhuman/flows/ops.rs`:
- Around line 2669-2670: Update the adjacent “flows_update: persisting changes”
event to include both effective_require_approval and side_effect_forced
alongside has_expected. Use the values returned by enforce_side_effect_approval,
preserving the existing branch/state-transition diagnostics and making both
approval outcomes explicitly auditable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34b87662-34c8-4860-a5a8-e6b37ea84ccc
📒 Files selected for processing (4)
src/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/tinyflows/caps.rssrc/openhuman/tinyflows/tests.rs
…te (addresses @coderabbitai on ops.rs:2669) Nitpick from CodeRabbit review on PR tinyhumansai#5000: the flows_update persistence log recorded has_expected but not the security-sensitive effective approval decision, making it harder to audit Rule 2 forcing after the fact from logs alone.
…ses @coderabbitai on caps.rs:2650) CodeRabbit suggestion: the Deny branch on the Composio tool_call approval gate returned without a flow-level diagnostic, unlike the sibling native oh: tool_call Deny branch a few lines up. Logs slug + tier decision only (no reason/args), matching the redaction convention used elsewhere in this file.
…(addresses @coderabbitai on ops_tests.rs:4382) CodeRabbit flagged that flows_update_forces_require_approval_when_adding_side_effect_nodes only covers the direct ops::flows_update API, with no JSON-RPC-layer case proving the openhuman.flows_update controller (schema + handler wiring) forces require_approval=true when require_approval: false is passed alongside a graph that gained an outbound side-effect node, and that the forced value is what's actually persisted (not just echoed back).
|
Addressed the CodeRabbit nitpick on All three CodeRabbit findings addressed:
Verification: No changes to the security posture: tier gate still runs before curation on the Composio branch, still classified |
Summary
C1 security fix — the Composio
tool_callbranch inOpenHumanTools::invoke(src/openhuman/tinyflows/caps.rs) reached the approval gate (intercept_audited) without ever consulting the user's autonomy tier. Every other acting node path — nativeoh:tools,http_request, andcode— goes throughenforce_node_tier_gate+gate_call_for_tierfirst. The Composio path skipped straight to curation + approval, so a read-only-tier user's flow could still fire an outbound Composio call (Slack, Gmail, etc.) unattended.This bypass compounded with a second gap:
flows_updatenever re-appliedflows_create's Rule 2 (forcerequire_approval: truewhen the graph contains an outbound side-effect node). So a flow saved read-only could later be edited to add a Composio/http_request/codenode while keepingrequire_approval: false— the update path trusted the caller's toggle unconditionally instead of re-deriving it from the new graph.Net effect: a flow could gain an outbound Composio node with
require_approval: falseand fire unattended even under areadonlyautonomy tier.Fix (two parts, one shared helper each)
1.
src/openhuman/tinyflows/caps.rs— tier-gate the Composio dispatch pathOpenHumanToolsnow carriessecurity: Arc<SecurityPolicy>(previously onlyconfig).is_curated_flow_tool), the Composio branch now callsenforce_node_tier_gate(&self.security, CommandClass::Network, "tool_call")— sameCommandClass::Networkclassificationhttp_requestuses, since a Composio action is a third-party API call.ApprovalGate::intercept_auditedcall is replaced with thegate_call_for_tierpattern copied fromOpenHumanHttp::request, so a Supervised-tierPromptdecision correctly escalates past a flow's ownrequire_approval: falsedefault (closing the same Codex P1 gaphttp_request/codealready close).oh:branch's inlineSecurityPolicy::from_config(...)construction is replaced with&self.security(no behavior change, removes a redundant construction).OpenHumanToolsconstruction sites (build_capabilities, and thetools_adaptertest helper intests.rs) updated to passsecurity.2.
src/openhuman/flows/ops.rs— re-apply Rule 2 on update, not just createenforce_side_effect_approval(graph, caller_require_approval) -> (effective_require_approval, was_forced), next tograph_has_outbound_side_effect.flows_createrefactored to use it (behavior-preserving — same logs, same outcome).flows_updatenow calls the same helper on the effective graph (the one actually being persisted) and passeseffective_require_approval(not the caller's rawnew_require_approval) tostore::update_flow_graph, with the same "require_approval forced to true…" log lineflows_createalready emits.Toolkit-neutral throughout —
SLACK_SEND_MESSAGEis used only as a sample slug in the new test; no Slack/Gmail-specific logic was added.Failing-test-first
composio_tool_call_blocks_under_readonly_tier(caps.rs): constructsOpenHumanToolsunder aReadOnlypolicy and assertsinvoke("SLACK_SEND_MESSAGE", ...)returnsEngineError::CapabilitycarryingPOLICY_BLOCKED_MARKER. Before the fix,OpenHumanToolsdidn't even have asecurityfield — confirmed as a compile error against the pre-fix struct shape.flows_update_forces_require_approval_when_adding_side_effect_nodes(ops_tests.rs): creates a trigger-only flow withrequire_approval: false, then updates it to a graph with atool_callnode passingSome(false), and assertsrequire_approvalis forcedtruewith a matching log line. Confirmed failing pre-fix (thread ... panicked ... flows_update must force require_approval ...), passing post-fix.flows_update_does_not_force_require_approval_on_readonly_graph: a name-only update to a read-only graph must not forcerequire_approval— passes both before and after, proving the fix doesn't over-gate.Verification
GGML_NATIVE=OFF cargo test --lib openhuman::tinyflows::— 133 passed, 0 failed, 1 ignored.GGML_NATIVE=OFF cargo test --lib openhuman::flows::— 397 passed, 0 failed.http_request_node_tier_gate_blocks_readonly_allows_higher,code_node_tier_gate_blocks_readonly_allows_full,http_adapter_blocks_under_readonly_tier,prompt_decision_*,allow_decision_never_escalates,flows_create_outbound_node_forces_require_approval,graph_has_outbound_side_effect_*,draft_promote_*— all pass.GGML_NATIVE=OFF cargo check— clean.cargo fmt --check— clean.This touches the approval/security core, so the fix mirrors existing patterns exactly (
enforce_node_tier_gate+gate_call_for_tier, same ashttp_request/code) rather than introducing a new gating mechanism, and deliberately does not add any Block-tier behavior beyond whathttp_requestalready enforces for the sameCommandClass::Network.Test plan
cargo test --lib openhuman::tinyflows::greencargo test --lib openhuman::flows::greencargo checkcleancargo fmt --checkcleanSummary by CodeRabbit