fix(plugins): replace BLOCKED_MODULES deny-list with permission-gated allow-list#111
Merged
sreerevanth merged 1 commit intoJun 3, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces permission-based sandbox enforcement via allow-list import model. Replaces prior deny-list with explicit safe-module set plus permission-gated module groups. Adds ChangesPlugin Permission Enforcement Sandbox
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
Merged
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.
Owner
|
@anshul23102 can u please check these test pr tests and get back !! please |
anshul23102
force-pushed
the
fix/98-sandbox-blocked-modules
branch
from
8a351bb to
ae0812a
Compare
Contributor
Author
|
Branch rebased onto current `main` and lint failures resolved. The `Test & lint` CI was failing because:
All checks should be green on the new push. Please re-review when you get a chance. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_adapter_crewai.py (1)
62-67: ⚡ Quick winRestore
sys.modules["crewai"]after tests to avoid cross-test contamination.This global patch persists beyond this module and can make other tests order-dependent. Wrap injection in a fixture (or module setup/teardown) that restores prior state.
Proposed fix
import sys import types import uuid +import pytest @@ -_fake_crewai = types.ModuleType("crewai") -_fake_crewai.Agent = FakeAgent # type: ignore[attr-defined] -_fake_crewai.Crew = FakeCrew # type: ignore[attr-defined] -sys.modules["crewai"] = _fake_crewai +@pytest.fixture(autouse=True, scope="module") +def _patch_crewai_module(): + previous = sys.modules.get("crewai") + fake_crewai = types.ModuleType("crewai") + fake_crewai.Agent = FakeAgent # type: ignore[attr-defined] + fake_crewai.Crew = FakeCrew # type: ignore[attr-defined] + sys.modules["crewai"] = fake_crewai + try: + yield + finally: + if previous is None: + sys.modules.pop("crewai", None) + else: + sys.modules["crewai"] = previous🤖 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_adapter_crewai.py` around lines 62 - 67, The test currently injects a fake module into sys.modules by assigning _fake_crewai (with FakeAgent and FakeCrew) to sys.modules["crewai"] but never restores the original entry, causing cross-test contamination; update tests/test_adapter_crewai.py to wrap this injection in a fixture or module-level setup/teardown that saves the original = sys.modules.get("crewai"), sets sys.modules["crewai"] = _fake_crewai for the duration of the test(s), and restores the original (deleting the key if original was None) in the fixture teardown or teardown function so other tests are unaffected.
🤖 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.
Nitpick comments:
In `@tests/test_adapter_crewai.py`:
- Around line 62-67: The test currently injects a fake module into sys.modules
by assigning _fake_crewai (with FakeAgent and FakeCrew) to sys.modules["crewai"]
but never restores the original entry, causing cross-test contamination; update
tests/test_adapter_crewai.py to wrap this injection in a fixture or module-level
setup/teardown that saves the original = sys.modules.get("crewai"), sets
sys.modules["crewai"] = _fake_crewai for the duration of the test(s), and
restores the original (deleting the key if original was None) in the fixture
teardown or teardown function so other tests are unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a81e1ff9-eb33-40e7-b125-0a915cb03040
📒 Files selected for processing (3)
agentwatch/plugins/sandbox.pytests/test_adapter_crewai.pytests/test_sandbox.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_sandbox.py
- agentwatch/plugins/sandbox.py
Owner
|
@anshul23102 lookks like there is conflicts fix tht ill merge it asa u fix tht ready to merge |
…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
anshul23102
force-pushed
the
fix/98-sandbox-blocked-modules
branch
from
ae0812a to
dfc5ab8
Compare
Contributor
Author
|
@sreerevanth the conflicts are resolved. Rebased onto the latest main, which included the shell injection fix from PR #110. The deny-list to allow-list change in sandbox.py is clean and ready to merge. |
sreerevanth
added
help wanted
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.
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