fix(security): expand default allowed_commands and auto_approve (#2486)#2673
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExpands default allowed commands and auto-approve lists, raises default max_actions_per_hour to unlimited, adds a v3→v4 migration to merge new defaults and conditionally bump the action limit, adds a v4→v5 migration to remove write tools from auto-approve, updates migration runner to CURRENT_SCHEMA_VERSION = 5, and adds tests. ChangesAutonomy defaults expansion and migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/security/policy.rs (1)
179-179:⚠️ Potential issue | 🔴 Critical | 💤 Low valueFix rate-limit default mismatch: tool paths using
SecurityPolicy::default()still cap at 20/hr
SecurityPolicy::default()setsmax_actions_per_hour: 20, butAutonomyConfig’s default (default_max_actions_per_hour()inautonomy.rs) isu32::MAX(effectively unlimited). Production code also constructs the policy fromSecurityPolicy::default()instead of config-layer defaults:
src/openhuman/tools/schemas.rs(polymarket_execute) passesSecurityPolicy::default()intoPolymarketTool, andPolymarketTool::executeenforces rate limiting viasecurity.is_rate_limited()/security.record_action().src/openhuman/composio/client.rs(direct mode) passesSecurityPolicy::default()intoComposioTool, andComposioToolcallssecurity.enforce_tool_operation(...), which rate-limits viarecord_action().Use
SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir)(or equivalent) in these production call sites, or updateSecurityPolicy::default()to match the config default if that’s the intended global default.🤖 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/security/policy.rs` at line 179, SecurityPolicy::default() currently sets max_actions_per_hour to 20 whereas AutonomyConfig::default_max_actions_per_hour() uses u32::MAX, causing production call sites to be unintentionally rate-limited; fix by replacing uses of SecurityPolicy::default() in the production tool instantiations with SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir) (or otherwise constructing from the AutonomyConfig) so PolymarketTool (polymarket_execute) and ComposioTool (composio client direct mode) receive policy values that match AutonomyConfig defaults, or alternatively update SecurityPolicy::default() to return the same default as default_max_actions_per_hour if a global default change is intended.
🤖 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.
Outside diff comments:
In `@src/openhuman/security/policy.rs`:
- Line 179: SecurityPolicy::default() currently sets max_actions_per_hour to 20
whereas AutonomyConfig::default_max_actions_per_hour() uses u32::MAX, causing
production call sites to be unintentionally rate-limited; fix by replacing uses
of SecurityPolicy::default() in the production tool instantiations with
SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir) (or
otherwise constructing from the AutonomyConfig) so PolymarketTool
(polymarket_execute) and ComposioTool (composio client direct mode) receive
policy values that match AutonomyConfig defaults, or alternatively update
SecurityPolicy::default() to return the same default as
default_max_actions_per_hour if a global default change is intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2201ad39-da37-4dea-ae7b-b34f49f51556
📒 Files selected for processing (6)
src/openhuman/config/schema/autonomy.rssrc/openhuman/migrations/expand_autonomy_defaults.rssrc/openhuman/migrations/mod.rssrc/openhuman/migrations/mod_tests.rssrc/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rs
…#2486 Agents were silently failing because the 18-command default allowlist was too narrow for everyday development workflows. Add 21 commonly safe commands across three categories: - Build tools: pnpm, yarn, make, cmake - Read-only inspection tools (low-risk): sort, uniq, diff, which, uname, basename, dirname, tr, cut, realpath, readlink, stat, file - Filesystem mutation tools (medium-risk, require approval in Supervised mode): mkdir, touch, cp, mv, ln High-risk command executors (python, node, bash, env, xargs, awk, etc.) and redirect-capable commands (tee) remain blocked regardless of the allowlist, as enforced by command_risk_level() and is_command_allowed(). Mirrors the change in both SecurityPolicy::default() (policy.rs) and default_allowed_commands() (autonomy.rs) so they stay in sync. Adds test default_policy_allowed_commands_expanded to cover the new entries, including risk-level assertions for the medium-risk group. [security] [policy]
…i#2486 The 5-tool default auto_approve list caused unnecessary approval prompts for common workspace-scoped operations in Supervised mode. Add four commonly safe tools: - glob, grep — read-only workspace search; no side effects - file_write, edit_file — workspace-scoped writes; the workspace policy boundary already constrains the blast radius, so per-call prompts add friction without proportional safety benefit High-risk tools (shell, web_fetch, etc.) remain outside the auto_approve list and continue to trigger the approval workflow as designed. The always_ask list can override for any tool that a user wants to force-ask. Adds test config_default_auto_approve_includes_expanded_tools to verify both the pre-existing entries and the four new ones are present. [security] [policy]
…anded defaults mkdir is now in the default allowlist (tinyhumansai#2486), so the assertion flips from !allowed to allowed. The redirect-blocked and ls-allowed assertions remain unchanged.
YellowSnnowmann
force-pushed
the
fix/2486-expand-security-defaults
branch
from
086befe to
7d9d354
Compare
coderabbitai
Bot
added
the
feature
…ith AutonomyConfig default (addresses @coderabbitai on policy.rs:264) SecurityPolicy::default() had max_actions_per_hour: 20, which diverged from AutonomyConfig::default_max_actions_per_hour() (u32::MAX since this PR bumped it). The mismatch meant that policies built via SecurityPolicy::default() — used by ComposioTool and PolymarketTool call sites — would rate-limit after 20 actions while policies derived from AutonomyConfig::from_config() were effectively unlimited. Change the default to u32::MAX to restore consistency. The rate-limiter check is `count <= max`, so u32::MAX is functionally infinite without requiring an Option sentinel on the field type.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Solid migration work and good test coverage overall. One security-design decision needs explicit maintainer sign-off before merge; a couple of smaller items to address.
Security concern — file_write and edit_file in auto_approve
ApprovalManager::needs_approval() (src/openhuman/approval/ops.rs:92) is the gating function:
// ReadOnly blocks everything — handled elsewhere; no prompt needed.
if self.autonomy_level == AutonomyLevel::ReadOnly { return false; }
// auto_approve skips the prompt.
if self.auto_approve.contains(tool_name) { return false; }
// Default: supervised mode requires approval.
trueAdding file_write and edit_file to auto_approve means Supervised mode no longer prompts for file writes. ReadOnly mode is unaffected (writes are blocked at the tool layer, not the approval gate — the comment on line 82 confirms this). But Supervised mode's documented contract is "ask before edit" — that contract is broken for these two tools.
The PR description explicitly argues this is acceptable ("workspace boundary already constrains the blast radius"), but that's a design decision that changes user-visible behaviour for everyone in Supervised mode. It deserves explicit maintainer sign-off in the PR, not just a comment in the diff.
Suggested resolution: either (a) keep file_write/edit_file off auto_approve for now and file a separate PR documenting the change to Supervised mode semantics, or (b) add a note to the PR body confirming the intentional Supervised-mode behaviour change and have a maintainer explicitly approve it.
make in allowed_commands
make is a command runner, not just a build tool — a make target can shell out to anything. On the allowed list it isn't classified as Install, so a compromised or malicious Makefile can run arbitrary commands (including network calls) without triggering the always-ask Install gate. This is qualitatively different from pnpm/yarn/cmake, which install packages or configure build graphs.
Not a blocker, but worth a doc comment next to the make entry explaining the deliberate trade-off (e.g. "common development workflow; targets must be approved in Supervised mode as medium-risk writes").
Minor — cp/mv/ln classification confirmed correct
policy.rs:536 already has "touch" | "mkdir" | "mv" | "cp" | "ln" => true for medium-risk. The command_risk_level test in policy_tests.rs pins that. ✓
Minor — NEW_COMMANDS comment inaccuracy
/// These were added to the code default in PR #2500 but are absent from any
/// `config.toml` written before that change.
const NEW_COMMANDS: &[&str] = &[
..., "date", "dir", "type", "where", "findstr", "more",
];date, dir, type, where, findstr, more appear without + in the autonomy.rs diff, meaning they were present before this PR. If they pre-date PR #2500 too, the comment is inaccurate. Functionally harmless (the additive guard prevents duplicates), but the comment will mislead future readers. Worth a quick git log -S '"date".into()' to verify — if they were there before PR #2500, update the comment to name the correct PR or say "may already be present in older configs; merge is a no-op if so."
What's correct
- Migration is additive-only (contains-check before every push) and fully idempotent. ✓
- Save-failure rollback (
config.schema_version = previous_version; return;) prevents a half-applied migration from persisting a bumped version. ✓ max_actions_per_hourbump is gated on exact equality withOLD_DEFAULT_MAX_ACTIONS_PER_HOUR = 20, so any deliberate customisation is preserved. ✓no_op_on_fresh_install_defaultscorrectly fires!stats.max_actions_bumpedbecauseConfig::default()now initialisesmax_actions_per_hour = u32::MAX, not 20. ✓- Migration integration tests (
run_pending_expands_autonomy_defaults_from_v3,run_pending_v3_to_v4_preserves_custom_max_actions) confirm the end-to-end path includingconfig.save(). ✓ supervised_still_enforces_command_allowlistupdated to reflectmkdirnow being allow-listed. ✓- CI green, no merge conflicts.
Resolve the file_write/edit_file auto-approve question and this is ready.
Resolved the issue : 9922974 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/migrations/expand_autonomy_defaults.rs`:
- Line 49: The change in expand_autonomy_defaults.rs only stops adding
file-write tools for new migrations but does not remove previously-granted
file_write/edit_file from workspaces already at schema_version == 4; add a new
migration (4→5) that scans workspaces with schema_version == 4 and removes or
re-gates the write-capability entries (file_write, edit_file) from their
autonomy/tool lists, implement this in the new migration module (e.g.,
migrate_v4_to_v5 with an apply_migration or run function) and ensure it targets
the same data structures modified by expand_autonomy_defaults.rs (autonomy/tool
config fields and any constants like NEW_AUTO_APPROVE_TOOLS), then add an
integration test that creates a workspace at schema_version 4 with
file_write/edit_file present and verifies the new migration strips them and
bumps schema_version to 5.
🪄 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: 0c9bef80-eaae-4d67-bbc2-1457c5f13d67
📒 Files selected for processing (3)
src/openhuman/config/schema/autonomy.rssrc/openhuman/migrations/expand_autonomy_defaults.rssrc/openhuman/migrations/mod_tests.rs
💤 Files with no reviewable changes (1)
- src/openhuman/config/schema/autonomy.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/migrations/expand_autonomy_defaults.rs (1)
80-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't widen intentionally restrictive v3 configs.
These loops add missing entries for every
schema_version == 3config, so a user who deliberately kept a narrowerallowed_commandsorauto_approvelist still gets extra permissions here. That also changes prompt behavior forglob/grep, becauseApprovalManager::needs_approval()skips prompts for anything inauto_approve. Please only expand when the persisted lists still match the legacy defaults (or at least haven't removed legacy entries), and add a regression test for a narrowed v3 config.🤖 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/migrations/expand_autonomy_defaults.rs` around lines 80 - 102, The migration currently unconditionally adds NEW_COMMANDS and NEW_AUTO_APPROVE_TOOLS to every schema_version == 3 config; change it to only merge when the persisted lists still equal the legacy v3 defaults (or at least contain all legacy entries) so deliberately-narrowed v3 configs are not widened—check config.autonomy.allowed_commands and config.autonomy.auto_approve against the legacy default sets before pushing NEW_* items, and leave them untouched if the user removed legacy entries; update the migration logic around the loops that reference NEW_COMMANDS/NEW_AUTO_APPROVE_TOOLS and config.autonomy.allowed_commands/auto_approve, and add a regression test that loads a narrowed v3 config (missing legacy defaults) and asserts the migration does not add the new entries and that ApprovalManager::needs_approval() behavior remains unchanged.
🤖 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.
Outside diff comments:
In `@src/openhuman/migrations/expand_autonomy_defaults.rs`:
- Around line 80-102: The migration currently unconditionally adds NEW_COMMANDS
and NEW_AUTO_APPROVE_TOOLS to every schema_version == 3 config; change it to
only merge when the persisted lists still equal the legacy v3 defaults (or at
least contain all legacy entries) so deliberately-narrowed v3 configs are not
widened—check config.autonomy.allowed_commands and config.autonomy.auto_approve
against the legacy default sets before pushing NEW_* items, and leave them
untouched if the user removed legacy entries; update the migration logic around
the loops that reference NEW_COMMANDS/NEW_AUTO_APPROVE_TOOLS and
config.autonomy.allowed_commands/auto_approve, and add a regression test that
loads a narrowed v3 config (missing legacy defaults) and asserts the migration
does not add the new entries and that ApprovalManager::needs_approval() behavior
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ac96ede-dbf1-4b20-b418-b0c95624e73e
📒 Files selected for processing (5)
src/openhuman/config/schema/autonomy.rssrc/openhuman/migrations/expand_autonomy_defaults.rssrc/openhuman/migrations/mod.rssrc/openhuman/migrations/mod_tests.rssrc/openhuman/migrations/remove_write_auto_approve.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — expand default allowed_commands and auto_approve (#2486)
Solid migration work. The additive-only merge logic is correct, idempotency is well-tested, and the v4→v5 rollback for write tools shows good follow-through on the CodeRabbit feedback.
One issue needs fixing before this can land.
| Area | Files | Verdict |
|---|---|---|
| Config defaults | autonomy.rs |
Clean — glob/grep additions are safe |
| Migration v3→v4 | expand_autonomy_defaults.rs |
Clean — additive merge, good guards |
| Migration v4→v5 | remove_write_auto_approve.rs |
Clean — strips write tools correctly |
| Migration wiring | mod.rs |
Clean — sequential gating, proper rollback on save failure |
| Integration tests | mod_tests.rs |
Clean — good coverage of v3→v5 path |
| Security policy | policy.rs |
Clean — mirrors autonomy defaults correctly |
| Policy tests | policy_tests.rs |
Bug — stale assertion, see inline |
PR description also says "5 → 9 tools" but code shows 5 → 7 (glob + grep only). Worth updating to match.
The test asserted file_write/edit_file were in default auto_approve, contradicting the migration that strips them. Split the assertion to verify read-only tools (glob, grep) are included and write tools are explicitly excluded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `always_ask` field was removed from `AutonomyConfig` in tinyhumansai#2706 (ApprovalGate consolidation). Migration tests still referenced it, causing compilation failures in CI.
3 participants
Summary
default_allowed_commandsfrom 18 → 40 entries: adds build tools (pnpm,yarn,make,cmake), read-only inspection tools (sort,uniq,diff,which,uname,basename,dirname,tr,cut,realpath,readlink,stat,file), and workspace mutation tools (mkdir,touch,cp,mv,ln).default_auto_approvefrom 5 → 9 tools: addsglob,grep(read-only search),file_write,edit_file(workspace-scoped writes).max_actions_per_hourfrom the old hard-coded20tou32::MAXfor users still on that default (code default was changed to unlimited in feat(app): UI control for max_actions_per_hour (#2486) #2500 but persisted configs kept20).No high-risk commands (python, node, bash, curl, sed, awk, xargs) were added. Filesystem mutation commands (
mkdir,touch,cp,mv,ln) are classified as medium-risk and require approval in Supervised mode.Test plan
cargo test --lib openhuman::security::policy— 112 passcargo test --lib openhuman::migrations— 51 passcargo fmt --check— cleanopenhuman.security_policy_infoshowsallowed_cmds=40,max_actions/hr=4294967295openhuman.config_getshows migration applied to real user config (commands_added=28 tools_added=4 max_actions_bumped=true)Closes #2486
Summary by CodeRabbit
New Features
Configuration Updates
Behavior Changes
Tests