Skip to content

fix: prevent command executors from bypassing security policy#2013

Merged
senamakel merged 3 commits into
tinyhumansai:mainfrom
Yong-yuan-X:fix/security-command-interpreter-policy
May 18, 2026
Merged

fix: prevent command executors from bypassing security policy#2013
senamakel merged 3 commits into
tinyhumansai:mainfrom
Yong-yuan-X:fix/security-command-interpreter-policy

Conversation

@Yong-yuan-X
Copy link
Copy Markdown
Contributor

@Yong-yuan-X Yong-yuan-X commented May 17, 2026

Summary

  • Blocks known command-executor tools from bypassing the security policy when explicitly added to allowed_commands.
  • Treats interpreters and shell bridges like xargs, awk, perl, python-family commands such as python, python3, and python3.12, ruby, bash, sh, and env as high-risk command executors.
  • Normalizes command names across Unix paths, Windows paths, and .exe suffixes before policy checks.
  • Adds regression coverage for issue Security: Command allowlist missing xargs, awk, perl, python, ruby (sandbox escape) #1923, including custom allowlist bypass attempts.

Problem

Issue #1923 identifies that the command allowlist can be weakened if dangerous command-executor tools are explicitly allowlisted. Tools such as xargs, awk, perl, Python-family interpreters, ruby, shell binaries, and env can execute arbitrary commands or code, so allowing them by command name alone creates policy bypass paths.

Solution

  • Added command-name normalization in src/openhuman/security/policy.rs.
  • Added hard-deny detection for known command executors before argument-specific allowlist checks.
  • Marked those command executors as CommandRiskLevel::High.
  • Added tests proving these commands remain blocked even when present in a custom allowed_commands list.

Submission Checklist

  • Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
  • Diff coverage ≥ 80% - Focused Rust diff-cover passed with 100% changed-line coverage for this PR. Full cargo llvm-cov -p openhuman was attempted but did not complete successfully before final report generation.
  • Coverage matrix updated - N/A: security policy hardening only, no feature row added/removed/renamed.
  • All affected feature IDs from the matrix are listed in the PR description under ## Related - N/A: no coverage-matrix feature ID.
  • No new external network dependencies introduced (mock backend used per Testing Strategy)
  • Manual smoke checklist updated if this touches release-cut surfaces - N/A: no release manual smoke surface changed.
  • Linked issue closed via Closes #NNN in the ## Related section

Impact

This is a Rust core security-policy hardening change. It affects command validation for CLI/RPC/agent flows that call SecurityPolicy::is_command_allowed() or validate_command_execution().

No migration or frontend behavior changes are expected. Custom configurations that previously allowed these interpreter/bridge commands will now have them blocked by policy.

Related


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

Linear Issue

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

Commit & Branch

  • Branch: fix/security-command-interpreter-policy
  • Commit SHA: c2f70bb0ccaeb8af85effc7ea2e4e6fa8a1e4554

Validation Run

  • N/A: pnpm --filter openhuman-app format:check — no app workspace changes.
  • N/A: pnpm typecheck — no TypeScript changes.
  • Focused tests: cargo test --manifest-path Cargo.toml --lib openhuman::security::policy::tests:: -- --nocapture
  • Rust fmt/check: cargo fmt --check --manifest-path Cargo.toml; cargo check --manifest-path Cargo.toml --lib
  • N/A: Tauri fmt/check — no app/src-tauri changes.
  • Focused diff coverage: cargo llvm-cov test -p openhuman --manifest-path Cargo.toml --lib --lcov --output-path lcov-core.info -- openhuman::security::policy::tests::; diff-cover lcov-core.info --compare-branch=origin/main --fail-under=80 — passed, 100% changed-line coverage.

Validation Blocked

  • command: cargo llvm-cov -p openhuman --lcov --output-path lcov-core.info
  • error: Full core coverage run attempted but did not complete successfully before final report generation.
  • impact: Focused Rust policy tests and focused diff coverage passed; full CI coverage should be relied on for merged all-target coverage.

Behavior Changes

  • Intended behavior change: command executors/interpreters are blocked even when included in allowed_commands.
  • User-visible effect: unsafe shell command requests using these executors will be rejected by the security policy.

Parity Contract

  • Legacy behavior preserved: existing safe allowlisted commands such as git status, find . -name, pipes between safe commands, and env-var prefixes for safe commands remain allowed.
  • Guard/fallback/dispatch parity checks: existing policy module tests pass; new checks run through the same SecurityPolicy guard path.

Duplicate / Superseded PR Handling

  • Duplicate PR(s): N/A
  • Canonical PR: N/A
  • Resolution (closed/superseded/updated): N/A

Summary by CodeRabbit

  • Security
    • Strengthened shell command execution enforcement with enhanced detection and blocking of command executor binaries (interpreters and shells).
    • Improved prevention of embedded command patterns and command injection attempts through stricter risk classification and allowlist validation.

Review Change Stack

@Yong-yuan-X Yong-yuan-X requested a review from a team May 17, 2026 16:23
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 79979f62-65fb-462e-b885-ed093560b813

📥 Commits

Reviewing files that changed from the base of the PR and between c2f70bb and 4c6ecc0.

📒 Files selected for processing (2)
  • src/openhuman/cron/scheduler_tests.rs
  • src/openhuman/tools/impl/system/shell.rs

📝 Walkthrough

Walkthrough

This PR adds command-name normalization helpers, detects common interpreter/utility "command executors", marks them high-risk in command_risk_level, rejects them early in is_args_safe, and updates is_command_allowed to use the basename helper. Tests ensure executors remain blocked even with custom allowlists.

Changes

Command Executor Blocking in Security Policy

Layer / File(s) Summary
Helper functions for command normalization and executor detection
src/openhuman/security/policy.rs
Introduce command_basename, normalize_executable_name, is_python_invocation, and is_command_executor helpers to extract and classify dangerous command interpreters and utilities.
Command executor risk classification
src/openhuman/security/policy.rs
Update SecurityPolicy::command_risk_level to use the normalization helpers and mark command executors as high-risk while preserving previous high-risk checks.
Refactor is_command_allowed to use basename helper
src/openhuman/security/policy.rs
Derive the allowlist comparison key via the new command_basename helper instead of manual rsplit('/') parsing.
Early args rejection for executors
src/openhuman/security/policy.rs
Add an early return in SecurityPolicy::is_args_safe that rejects commands whose normalized base is classified as a command executor before per-command checks run.
Tests for command executor blocking behavior
src/openhuman/security/policy_tests.rs
Add command_risk_high_for_command_executors and custom_allowlist_cannot_enable_command_executors tests asserting executors are high risk and cannot be enabled via custom allowlists.
Test updates: scheduler and shell tests
src/openhuman/cron/scheduler_tests.rs, src/openhuman/tools/impl/system/shell.rs
Adjust test imports/permissions and tighten allowed command sets and executed command strings in existing tests to align with the updated policy behavior.
sequenceDiagram
  participant Caller as "Caller"
  participant Policy as "SecurityPolicy"
  participant Basename as "command_basename"
  participant Normalize as "normalize_executable_name"
  participant ExecCheck as "is_command_executor"
  participant Risk as "command_risk_level"
  participant Args as "is_args_safe"

  Caller->>Policy: submit command line
  Policy->>Basename: extract basename
  Basename->>Normalize: normalize (lowercase, strip .exe)
  Normalize->>ExecCheck: check executor list / python variants
  ExecCheck->>Risk: flag High risk if executor
  Risk->>Args: cause early rejection if executor
  Args->>Policy: return allowed/denied
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I nibble code at dawn's first light,
Helpers trim names, make threats take flight.
Xargs and perl and python too,
Can't slip past this rabbit crew —
Guards up, allowlists held tight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main objective of the PR: preventing command executors (interpreters and shell bridges) from bypassing the security policy.
Linked Issues check ✅ Passed The PR addresses all coding requirements from issue #1923: command executors are classified as high-risk, command names are normalized across Unix/Windows paths and .exe suffixes, and hard-deny detection is implemented in is_command_allowed() and is_args_safe().
Out of Scope Changes check ✅ Passed All changes are directly scoped to the security policy hardening objective: normalizing command names, classifying executors as high-risk, blocking them in allowlist checks, and adding focused regression test coverage for issue #1923.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/security/policy.rs`:
- Around line 193-201: The is_python_command function misses Windows
pythonw<version> names (e.g., pythonw3, pythonw3.12); update the suffix check in
is_python_command (after calling normalized_command_name) to treat a suffix that
starts with a digit OR starts with 'w' followed by a digit as a versioned python
variant. Concretely, in is_python_command replace the current
strip_prefix("python")...is_some_and(|ch| ch.is_ascii_digit()) logic with a
predicate that returns true when the first suffix char is a digit OR when the
first char == 'w' and the second char is a digit, so python3, python312,
pythonw3, pythonw3.12, etc. are all detected.
🪄 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: dcd2b1cd-0a81-4c51-a352-482c97c62675

📥 Commits

Reviewing files that changed from the base of the PR and between f9de38d and 95aa364.

📒 Files selected for processing (2)
  • src/openhuman/security/policy.rs
  • src/openhuman/security/policy_tests.rs

Comment thread src/openhuman/security/policy.rs
coderabbitai[bot]
coderabbitai Bot previously approved these changes May 17, 2026
@senamakel senamakel merged commit f471d53 into tinyhumansai:main May 18, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Command allowlist missing xargs, awk, perl, python, ruby (sandbox escape)

2 participants