fix(flows): close two authorization boundaries in flow-run tools

[graycyrus]

Summary

• Closes two authorization boundaries in the flow-run tool surface, both of which the code's own comments already half-acknowledged.
• flow_memory_remember no longer trusts a model-supplied flow_id outside a workflow run — a prompt-injected chat turn could poison another flow's private dedup memory.
• cancel_flow_run now reports external_effect() == true so it parks for approval instead of cancelling any run unapproved — that is the real boundary — plus an ownership check as defence-in-depth (see the honest scoping note below).
• Corrects a registration comment in tools/ops.rs that asserted an invariant the code did not hold.

Problem

T-M2 — cross-flow memory poisoning from chat. Inside a run, FlowMemoryRememberTool resolves the namespace-governing flow id from the trusted TrustedAutomation { Workflow } turn origin and ignores the arg — airtight. But when trusted_flow_id() returns None, which is every chat/orchestrator turn, it fell back to the model-supplied flow_id and wrote into that flow's namespace.

The registration comment in tools/ops.rs claimed "there is no code path by which either tool can address a namespace other than the calling flow's own." That was true only inside runs.

Concretely: a prompt-injected chat turn calls flow_memory_remember { flow_id: "<digest-flow-id>", key: "sent_item_42", … } and the digest flow's dedup memory now says item 42 was already sent, so it silently skips it forever. The tool has no external_effect, so this never parked for approval.

T-M3 — cancel_flow_run had no ownership check. It cancelled an arbitrary run_id (its own doc comment said so), reported external_effect() == false so the approval gate never parked it, and was hidden only on the two flows_build paths — not on orchestrator delegation or main chat, which also carry the tool. Injected content could therefore cancel any in-flight or approval-parked automation.

Solution

T-M2 — outside a trusted workflow run the write is refused outright rather than routed to an arg that cannot be distinguished from an attacker's. There is no legitimate chat-side use case for writing another flow's private namespace, and refusing is the only fail-closed option that does not require inventing an ownership proof. The registration comment is corrected to describe the real invariant. flow_memory_recall is untouched — it stays read-only, and its scope: "flows" cross-flow read is deliberate.

A caller audit found nothing depending on the removed fallback; tinyflows's own memory-node adapter already used this fail-closed pattern.

T-M3 — two changes, of unequal weight:

1. external_effect() == true — so ApprovalSecurityMiddleware parks it wherever a gate exists. This is the actual authorization boundary. Pinned by an ApprovalGate integration test proving the call parks and TTL-denies rather than auto-executing.
2. Ownership check — the tool now takes { flow_id, run_id }, resolves the run's actual flow_id, and refuses on mismatch, mirroring resume_flow_run/flows_resume's existing shape rather than inventing a new one.

Honest scoping: the ownership check narrows the attack, it does not close it

Review raised this and it is worth stating plainly rather than leaving the summary to imply otherwise. list_flows takes no arguments, is PermissionLevel::None and external_effect() == false; list_flow_runs returns flow_id inline for every run. Chaining the two reconstructs the full run_id → flow_id map, and both are available on exactly the surfaces cancel_flow_run is. So a caller that can reach cancel_flow_run at all can simply look up the correct flow_id first — the ownership check costs an attacker one extra tool round-trip, nothing more.

It is still worth having: it stops accidental cross-flow cancellation, such as a model reusing a stale or hallucinated run_id. But the approval-gate parking is what actually stops an unapproved cancel, and the same defeatable pattern already exists unchanged in resume_flow_run, so this PR mirrors precedent rather than introducing a new false sense of security. The in-code comments were already accurate on this point; this section brings the PR summary in line with them.

Deliberate non-change reviewers should weigh

Both flows_build hide-lists are left unchanged. Hardening the tool makes it safe to unhide on the copilot path — but unhiding is a capability expansion, not a security fix: it would newly let an authoring turn tear down a run the user started. That product decision has not been taken, and hardening is not a reason to take it implicitly. A user can still cancel from the Runs rail. The entry can be dropped whenever you decide you want it; this PR is what makes doing so safe. It stays hidden on the headless path regardless, since a Cli-trusted origin auto-allows external_effect tools.

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
• Diff coverage ≥ 80% — refusal paths, the still-working in-run write, ownership mismatch, and a real ApprovalGate park are all covered; cargo test --lib openhuman::flows = 554 passed, 0 failed
• Coverage matrix updated — N/A: security hardening of existing tools, no feature rows added/removed/renamed
• All affected feature IDs from the matrix are listed under ## Related — N/A: no matrix feature rows affected
• No new external network dependencies introduced
• Manual smoke checklist updated if this touches release-cut surfaces — N/A: agent-tool surface only
• Linked issue closed via Closes #NNN — N/A: found by code review, no tracking issue filed yet

Impact

• Runtime/platform: Rust core agent-tool surface only.
• Behaviour (breaking for agents): cancel_flow_run's schema gains a required flow_id. Any caller passing only run_id now gets a clear parameter error. flow_memory_remember called outside a run now errors instead of writing.
• Security: both changes are fail-closed. The ApprovalGate test proves the now-external-effect cancel_flow_run genuinely parks and times out to Deny rather than auto-executing.
• Migration: none. No schema change, no Cargo.toml/Cargo.lock change.

Related

• Closes: N/A
• Follow-up PR(s)/TODOs: decide whether to unhide cancel_flow_run on the copilot path (now safe, deliberately deferred). The stale-approval-after-graph-swap gap (a run parked on the old graph config resuming against a rewritten node) is a separate stacked PR.
• Merge order: overlaps src/openhuman/flows/ops.rs + ops_tests.rs with the resume-lifecycle PR, and builder_tools.rs + builder_tools_tests.rs with the contract-drift PR. Merge the resume-lifecycle PR first, then this, then contract-drift.

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: fix/flows-authorization-boundaries
• Commit SHA: 9e59c8cca

Validation Run

• pnpm --filter openhuman-app format:check — N/A, no frontend files changed
• pnpm typecheck — N/A, no TypeScript changed
• Focused tests: GGML_NATIVE=OFF cargo test --lib openhuman::flows → 554 passed, 0 failed
• Rust fmt/check (if changed): GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml clean
• Tauri fmt/check (if changed): N/A, app/src-tauri untouched

Validation Blocked

• command: N/A
• error: N/A
• impact: N/A

Behavior Changes

• Intended behavior change: flow-scoped memory writes require a trusted workflow run origin; run cancellation requires proving which flow owns the run and parks for approval.
• User-visible effect: an agent asked to cancel a run must name the owning flow, and the cancel surfaces an approval card where a gate exists.

Parity Contract

• Legacy behavior preserved: the in-run flow_memory_remember path is byte-for-byte unchanged and still pinned by remember_ignores_mismatched_flow_id_arg_inside_trusted_workflow_run; flow_memory_recall is untouched.
• Guard/fallback/dispatch parity checks: both flows_build hide-lists are unchanged and still pinned by their existing contents tests, so no path silently gained a tool.

Duplicate / Superseded PR Handling

• Duplicate PR(s): none
• Canonical PR: this one
• Resolution: N/A

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aec2d08c-5c45-4130-aa16-9dab32168b46

📥 Commits

Reviewing files that changed from the base of the PR and between 46e9010 and 60ffd47.

📒 Files selected for processing (7)

• src/openhuman/approval/gate.rs
• src/openhuman/flows/builder_tools.rs
• src/openhuman/flows/builder_tools_tests.rs
• src/openhuman/flows/memory_tools.rs
• src/openhuman/flows/ops.rs
• src/openhuman/flows/ops_tests.rs
• src/openhuman/tools/ops.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[graycyrus]

Review follow-up: no code change, but I corrected this PR's own description, which overstated what the ownership check buys.

The summary implied the { flow_id, run_id } ownership check is what stops an unapproved cancel. It isn't. list_flows takes no arguments and is PermissionLevel::None/no-external-effect, and list_flow_runs returns flow_id inline for every run — so chaining the two reconstructs the full run_id → flow_id map, and both tools sit on exactly the surfaces cancel_flow_run does. A caller that can reach cancel_flow_run can just look the id up first. The check costs an attacker one extra round-trip.

The real boundary is external_effect() == true routing the call through the approval gate, which the new ApprovalGate test pins (parks, TTL-denies, never auto-allows). The ownership check remains worth having as defence-in-depth against accidental cross-flow cancellation — a model reusing a stale or hallucinated run_id — and it mirrors the same shape resume_flow_run already uses.

The in-code comments were already honest about this; the PR summary was the part that drifted. Fixed there rather than left to a reader to discover — overstating a security fix is its own kind of bug.

[greptile-apps]

graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[greptile-apps]

graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.