feat: tighten runtime policy + transport guards v2#2636
Merged
graycyrus merged 7 commits intoMay 26, 2026
Conversation
Mirrors the autonomy gate already enforced by file_write, edit_file, apply_patch, curl, http_request, pushover, csv_export, and the rest of the action-tool surface. Without this, an agent running under AutonomyLevel::ReadOnly could still execute arbitrary JavaScript via node_exec inline_code (and through it shell out to anything else), silently defeating the read-only guarantee that channel-bot deployments rely on. Guard fires before the existing rate-limit check so an unrelated tool denial cannot mask the autonomy-block error message.
…refixes
is_args_safe("find") previously rejected only -exec and -ok, leaving
-execdir and -okdir as a same-class bypass — they run an arbitrary
command per match with the working directory set to the match's
parent, identical execution semantics under a different option
spelling.
Separately, skip_env_assignments() let inline env-variable prefixes
(e.g. GIT_PAGER=<cmd> git log) pass the allowlist check. The bare
command is allowlisted but the prefix mutates how the downstream
binary executes — git, less, man, python and the dynamic loader all
have hook variables that turn an allowlisted binary into an arbitrary
command spawner. has_dangerous_env_prefix() encodes the set of names
that trigger this class (pager/editor/SSH/diff hooks, BASH_ENV,
PYTHONSTARTUP, LD_PRELOAD, LD_AUDIT, LD_LIBRARY_PATH, DYLD_*, PATH,
SHELL, IFS, PROMPT_COMMAND, and the PS1-PS4 family) and rejects the
whole segment when any of them are set.
Benign prefixes like TZ, LANG, LC_ALL, and arbitrary user-named
variables still pass — the allowlist deliberately enumerates only
the hook surfaces that are known to spawn subprocesses or alter
binary resolution.
Tests: dangerous_env_var_prefix_blocked covers every blocked name
plus the benign prefixes; command_argument_injection_blocked is
extended to cover -execdir and -okdir.
Browser.grantPermissions(displayCapture) bypasses Chromium's transient-activation requirement on navigator.mediaDevices .getDisplayMedia. With the pre-grant in place the embedded Meet and Slack Huddles webviews can initiate a desktop capture programmatically with no user gesture — the page never has to surface the screen-picker, so a malicious script inside the embedded origin can silently capture the desktop. Removing displayCapture from the pre-grant set restores the gesture gate. The user-visible flow stays the same: clicking Meet's "Present" / Slack's huddle share button still fires getDisplayMedia, Chrome's native screen-picker shows on the click, the user picks a source, the stream lands. The only behavior change is that the page can no longer call getDisplayMedia without a click. audioCapture, videoCapture, and clipboardReadWrite stay pre-granted for both origins — cam/mic auto-enumerate is the load-bearing piece for Meet's greenroom and Slack's huddle pre-flight (tinyhumansai#1022 / tinyhumansai#1054) and getUserMedia has its own permission model that the pre-grant doesn't compromise the same way.
Contributor
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR blocks dangerous inline env-prefix command forms and additional find execution flags, updates per-origin CDP permission pre-grants to remove desktop-capture for Meet/Slack while keeping media/clipboard, and serializes two MemoryTools tests with a global test lock. ChangesCommand Execution Security Hardening
Browser Permission Configuration
Test synchronization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_tests.rs`:
- Around line 762-767: The failing CI is due to rustfmt formatting mismatches on
the multi-line assert! calls using p.is_command_allowed in
src/openhuman/security/policy_tests.rs; reformat these assertions to match
rustfmt (either collapse the assert! into a single line or let rustfmt reflow
the string literal) and then run cargo fmt --all (or rustfmt on that file)
before committing so the assert!(!p.is_command_allowed(...)) lines conform to
the project formatting rules.
In `@src/openhuman/tools/impl/system/node_exec.rs`:
- Around line 155-159: The conditional early-return formatting around
self.security.can_act() is failing rustfmt; adjust the formatting of the return
expression that builds ToolResult::error (the return Ok(ToolResult::error(...))
line inside the if block) to match rustfmt expectations or simply run `cargo fmt
--all` so rustfmt rewrites the block correctly; ensure the if block uses
standard Rust formatting (brace placement and indentation) and the call to
ToolResult::error is aligned per rustfmt.
🪄 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: 452d9178-cbe5-4e48-bc45-48b2a921e235
📒 Files selected for processing (5)
app/src-tauri/src/cdp/session.rssrc/openhuman/security/policy.rssrc/openhuman/security/policy_tests.rssrc/openhuman/tools/impl/system/node_exec.rssrc/openhuman/tools/impl/system/npm_exec.rs
Collapse multi-line blocks rustfmt wanted single-line: - src/openhuman/security/policy_tests.rs:761 (assert) - src/openhuman/tools/impl/system/node_exec.rs:153 (ToolResult::error) - src/openhuman/tools/impl/system/npm_exec.rs:181 (ToolResult::error) - app/src-tauri/src/cdp/session.rs:539,560 (perms.extend_from_slice arrays)
…-tightening-v2 # Conflicts: # src/openhuman/tools/impl/system/node_exec.rs # src/openhuman/tools/impl/system/npm_exec.rs
Both async tests in `memory_tools::tools::put::tests` drive the process-global memory client (`active_memory_client()`), but they did not acquire `GLOBAL_MEMORY_TEST_LOCK` added in tinyhumansai#2649. Under cargo's default parallel test execution they raced with the new `memory::ops::*` tests that *do* hold the lock, picked up a polluted cached client, and intermittently tripped the PII-rejection guard on `tool-<name>` / `rule/<uuid>` writes — surfacing in CI as `memory_tools_put: store tool rule: document namespace/key cannot contain personal identifiers`. Closes that flake door by acquiring the same serial lock at the top of both tests, mirroring the pattern in `memory::ops::tool_memory::tests`.
graycyrus
reviewed
May 26, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — graycyrus
Solid security hardening PR. Three independent bypass classes closed with clean, well-scoped fixes.
Walkthrough
| Area | Files | What changed |
|---|---|---|
| Sandbox policy | security/policy.rs |
New DANGEROUS_ENV_PREFIXES blocklist (~35 env vars) + has_dangerous_env_prefix() check runs before skip_env_assignments() in is_command_allowed(). Blocks pager/editor/SSH/loader/shell-eval env overrides that would let an allowlisted binary spawn attacker-controlled subprocesses. Also extends find arg blocklist with -execdir / -okdir. |
| Embedded webview perms | cdp/session.rs |
displayCapture removed from Meet + Slack Browser.grantPermissions arrays. Chromium's transient-activation gate on getDisplayMedia now stays in force — screen-share still works via native picker on user click. |
| Tests | policy_tests.rs |
Comprehensive coverage: every blocked env name, case-insensitive matching, benign prefix passthrough, -execdir/-okdir blocking. |
| Test stability | memory_tools/tools/put.rs |
GLOBAL_MEMORY_TEST_LOCK added to serialize tests that share memory workspace state. |
Assessment
has_dangerous_env_prefix()— correctly handles multiple chained env assignments (SAFE=x GIT_PAGER=bad git log), validates first char is alphabetic/underscore (so shell quoting tricks like'GIT_PAGER'=xdon't false-positive), and uses case-insensitive matching. The linear scan over ~35 entries is fine for a per-command check.DANGEROUS_ENV_PREFIXESlist — good coverage of git hooks, pager/editor, loader overrides (LD_PRELOAD, DYLD_*), shell-eval vars (BASH_ENV, PROMPT_COMMAND, IFS), Python startup, and PATH/SHELL. TheGIT_EXTERNAL_FILTERentry doesn't correspond to a real git env var (filters use git config, not env), but it's harmless as an extra blocklist entry.findblocklist —-execdir/-okdirhave identical code-execution semantics to-exec/-ok, blocking them is correct.displayCaptureremoval — comments clearly explain the security rationale vs the preservedaudioCapture/videoCapture/clipboardReadWritegrants.- CI: all relevant checks pass. The Windows secrets ACL failure is unrelated (no secrets/ACL code touched).
- node_exec/npm_exec
can_act()guards (commit 1) are already merged into main via upstream merge — confirmed the guard exists at ~line 155 ofnode_exec.rson main.
No critical or major findings. Clean PR — moving to approval queue.
graycyrus
approved these changes
May 26, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
node_exec+npm_execnow honour thecan_act()gate that every other action tool already enforces.find -execdir/-okdirjoin the existing-exec/-okblock; inline env-var prefixes that mutate downstream subprocess execution (pager / editor / SSH / diff hooks + dynamic-loader overrides + shell-eval vars) are rejected at allowlist time.displayCaptureremoved from theBrowser.grantPermissionsset for Meet + Slack so Chromium's gesture gate ongetDisplayMediastays in force.Problem
The shipped sandbox + action-tool surface had three remaining gaps that the earlier transport tightening (#2331) did not cover:
Autonomy bypass on script-runner tools.
file_write,edit_file,apply_patch,curl,http_request,pushover,csv_export, and friends all short-circuit onAutonomyLevel::ReadOnlyviaself.security.can_act().node_execandnpm_execdid not — they only consulted the per-hour rate limiter. An agent configured for read-only operation (e.g. a Slack / iMessage / Discord assistant where the expected guarantee is "observe only, never act") could still run arbitrary JavaScript viainline_code, and through it arbitrary OS commands. Single prompt-injected message in a watched channel = full code execution on the host.findarg blocklist incomplete.is_args_safe("find")blocked-execand-ok. The semantically identical-execdirand-okdir(same per-match command execution, just with cwd set to the match's parent) were not blocked.findis in the default allowlist, so the gap was directly reachable.Inline env-var prefix bypass.
skip_env_assignments()stripped leadingKEY=valuetokens before the allowlist check. The bare command after the prefix (git log,git diff,cat, etc.) was allowlisted, but the prefix at exec time mutated the downstream binary's behaviour to spawn the assigned value as a subprocess:GIT_PAGER=<cmd> git log,GIT_SSH_COMMAND=<cmd> git fetch,LESSOPEN=<cmd> cat,LD_PRELOAD=<lib> any-allowed-binary,BASH_ENV=<rc> any-shell-call, etc.Browser.grantPermissions(displayCapture)bypassed Chromium's gesture gate. The embedded Meet + Slack Huddles webviews haddisplayCapturein their auto-grant set. Pre-granting it via CDP defeats the transient-activation requirement Chromium normally enforces onnavigator.mediaDevices.getDisplayMedia. A page inside the embedded origin could initiate a desktop capture programmatically — no click, no screen-picker. Auto-grant was originally added ([Feature] webview: Google Meet — full end-to-end parity with native app #1022 / feat(webview/gmeet): native cam/mic/screenshare + keep Meet routing in-app (#1022) #1054) to skip the cam/mic consent dialog; that justification holds foraudioCapture/videoCapturebut never applied todisplayCapture, which has its own native screen-picker flow that should run on user gesture.Solution
Three commits, scoped per-fix so each can be reverted independently:
feat(security/tools): require can_act for node_exec and npm_exec— three-line guard at the top of each tool'sexecute(), mirroring the pattern fromfile_write.rs:56-59. Fires before the existing rate-limit check so the autonomy-block error message is not masked by an unrelated rate-limit error.feat(security/policy): block find -execdir/-okdir and dangerous env prefixes— extends thefindblocklist with the two missing flags; addsDANGEROUS_ENV_PREFIXES(~30 names: pager / editor / SSH / diff hooks,BASH_ENV,ENV,PROMPT_COMMAND,PS1–PS4,PYTHONSTARTUP,PYTHONPATH,LD_PRELOAD,LD_AUDIT,LD_LIBRARY_PATH,DYLD_INSERT_LIBRARIES,DYLD_LIBRARY_PATH,DYLD_FORCE_FLAT_NAMESPACE,PATH,SHELL,IFS);has_dangerous_env_prefix()rejects the segment if any of those names appear. Benign prefixes (TZ,LANG,LC_ALL, arbitrary user-named) still pass. Allowlist intentionally enumerates hooks known to spawn subprocesses or alter binary resolution — no attempt to enumerate every env var ever assigned. Case-insensitive matcher sogit_pager=...andLd_PrElOaD=...are also rejected.fix(cdp/session): drop displayCapture from origin auto-grants— removes"displayCapture"from the Meet + Slack pre-grant arrays inapp/src-tauri/src/cdp/session.rs.audioCapture/videoCapture/clipboardReadWritestay pre-granted (these are load-bearing for greenroom cam/mic auto-grant per [Feature] webview: Google Meet — full end-to-end parity with native app #1022 / feat(webview/gmeet): native cam/mic/screenshare + keep Meet routing in-app (#1022) #1054 and don't compromise the gesture gate the same way). User-visible flow on click is unchanged: clicking Meet's "Present" / Slack's huddle share button still firesgetDisplayMedia, Chrome's native screen-picker shows on the click, the user picks a source, the stream lands. Only blocked path: the page callinggetDisplayMediaprogrammatically without a gesture.Submission Checklist
dangerous_env_var_prefix_blockedcovers every blocked env name plus the benign-prefix passthrough; existingcommand_argument_injection_blockedextended to cover-execdirand-okdir. 111 / 111 pass oncargo test --lib openhuman::security::policy::tests::.policy.rsand the policy module is covered by the extended + new tests;node_exec/npm_execguards mirror the existingcan_act()pattern that the surrounding tools already cover via their own ReadOnly tests;cdp/session.rschange is a deletion of three string literals from two arrays (no coverable code path added).docs/TEST-COVERAGE-MATRIX.md.## Related— same reason as above.docs/RELEASE-MANUAL-SMOKE.mdto smoke Meet "Present" + Slack huddle screen-share after this change is queued as a follow-up.Closes #NNNin the## Relatedsection — no public tracking issue (security-internal). Sentry / GHSA dashboard tracks the items off-PR.Impact
node_exec/npm_execnow refuse cleanly in ReadOnly (same error string as the other tools);find -execdir/-okdirnow refuse with the same code path as-exec/-ok; commands carrying a dangerous env prefix now refuse the same way an unallowlisted binary refuses;getDisplayMediafrom Meet + Slack pages now requires a click (matches Chromium's default for non-pre-granted origins).Related
docs/RELEASE-MANUAL-SMOKE.mdfor Meet "Present" + Slack huddle screen-share post-displayCapture-dropDANGEROUS_ENV_PREFIXESwhen adding new allowlisted commands insrc/openhuman/security/policy.rsAI Authored PR Metadata
Linear Issue
Commit & Branch
feat/runtime-policy-tightening-v2bf2b8bf0(tip; 3 commits in the series)Validation Run
pnpm --filter openhuman-app format:check— N/A: no TypeScript or app-workspace files touched (Rust core + Tauri shell only).pnpm typecheck— N/A: same reason.cargo test --lib openhuman::security::policy::tests::— 111 passed / 0 failed, including newdangerous_env_var_prefix_blockedand the extendedcommand_argument_injection_blocked.cargo check --manifest-path Cargo.tomlclean (pre-existing warnings onslack-backfillbinary unchanged);cargo fmt --checkclean on touched files.cargo check --manifest-path app/src-tauri/Cargo.tomlclean aftergit submodule update --init --recursive.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
node_exec/npm_execnow refuse with the standard "Action blocked: autonomy is read-only" error.getDisplayMediainside Meet / Slack now requires a user gesture (matches Chromium default).Parity Contract
TZ,LANG,LC_ALL, etc.) still pass the allowlist.findwith legitimate args (-name,-type,-maxdepth, etc.) still passes.can_act()guards mirror the existing pattern fromfile_write/edit_file/curl/http_requestexactly; reviewers can diff the new lines against any of those for visual parity.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Security Enhancements
Updates
Tests