fix: managed agents sandbox security (fixes #1426)#1442
Conversation
Add ManagedSandboxRequired exception for package installation safety Modify LocalManagedAgent to use compute providers for secure execution Implement compute-based tool execution routing for shell commands Add host_packages_ok safety opt-out flag for developer workflows Remove unused sandbox_type config field Add comprehensive tests for security functionality Security improvements: - Packages install in sandbox when compute provider attached - Raise exception when packages specified without compute/opt-out - Route execute_command, read_file, write_file, list_files through compute - Maintain backward compatibility with explicit opt-out flag Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds security controls to Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent / User Code
participant LMA as LocalManagedAgent
participant Compute as Compute Provider
participant Tools as Tool Bridge / Execution
Agent->>LMA: Initialize with packages & compute
LMA->>LMA: _install_packages()
alt Compute attached
LMA->>Compute: provision_compute() if needed
Compute-->>LMA: instance_id
LMA->>Compute: execute("pip install ...")
Compute-->>LMA: success
else No compute & host_packages_ok=False
LMA-->>Agent: Raise ManagedSandboxRequired
else No compute & host_packages_ok=True
LMA->>LMA: subprocess.run(pip install on host)
end
Agent->>LMA: Execute tool call (e.g., execute_command)
LMA->>LMA: _resolve_tools()
alt Compute attached
LMA->>Tools: _create_compute_bridge_tool()
Tools->>Compute: execute(command)
Compute-->>Tools: stdout/stderr
Tools-->>LMA: formatted result
else No compute
LMA-->>Tools: Use original tool implementation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the security of managed agents by enforcing sandboxing for package installations and bridging shell-based tools to run within a compute environment. It introduces a ManagedSandboxRequired exception and a host_packages_ok configuration to manage host-level execution risks. Review feedback highlights critical shell injection vulnerabilities in the command construction for file operations and pip installations, suggests optimizing tool resolution by moving imports out of loops, and identifies redundant imports.
| import asyncio | ||
|
|
||
| if tool_name == "read_file": | ||
| filepath = args[0] if args else kwargs.get("filepath", "") | ||
| if not filepath: | ||
| return "Error: No filepath specified" | ||
|
|
||
| command = f'cat "{filepath}"' | ||
|
|
||
| elif tool_name == "write_file": | ||
| filepath = args[0] if args else kwargs.get("filepath", "") | ||
| content = args[1] if len(args) > 1 else kwargs.get("content", "") | ||
| if not filepath: | ||
| return "Error: No filepath specified" | ||
|
|
||
| # Escape content for shell | ||
| import shlex | ||
| command = f'cat > "{filepath}" << "EOF"\n{content}\nEOF' | ||
|
|
||
| elif tool_name == "list_files": | ||
| directory = args[0] if args else kwargs.get("directory", ".") | ||
| command = f'ls -la "{directory}"' | ||
|
|
||
| else: | ||
| return f"Error: Unsupported bridged tool: {tool_name}" |
There was a problem hiding this comment.
The file operation tools are vulnerable to shell injection. Using simple string formatting with double quotes for filepath and directory can be bypassed if they contain quotes or shell metacharacters. Furthermore, the write_file implementation uses a heredoc with a fixed delimiter (EOF), which will fail or allow command injection if the content contains that delimiter. It is highly recommended to use shlex.quote() for all shell arguments and printf for writing content to avoid interpretation issues.
import shlex
if tool_name == "read_file":
filepath = args[0] if args else kwargs.get("filepath", "")
if not filepath:
return "Error: No filepath specified"
command = f"cat {shlex.quote(filepath)}"
elif tool_name == "write_file":
filepath = args[0] if args else kwargs.get("filepath", "")
content = args[1] if len(args) > 1 else kwargs.get("content", "")
if not filepath:
return "Error: No filepath specified"
# Safely write content using printf and shlex.quote
command = f"printf %s {shlex.quote(content)} > {shlex.quote(filepath)}"
elif tool_name == "list_files":
directory = args[0] if args else kwargs.get("directory", ".")
command = f"ls -la {shlex.quote(directory)}"
else:
return f"Error: Unsupported bridged tool: {tool_name}"| # No event loop, create one | ||
| asyncio.run(self.provision_compute()) | ||
|
|
||
| pip_cmd = "python -m pip install -q " + " ".join(f'"{pkg}"' for pkg in pip_pkgs) |
There was a problem hiding this comment.
The pip install command construction is vulnerable to shell injection. Wrapping package names in double quotes is insufficient if a package name contains a double quote or other shell-active characters. Use shlex.quote() for each package name to ensure they are safely handled by the shell.
| pip_cmd = "python -m pip install -q " + " ".join(f'"{pkg}"' for pkg in pip_pkgs) | |
| import shlex | |
| pip_cmd = "python -m pip install -q " + " ".join(shlex.quote(pkg) for pkg in pip_pkgs) |
| for name in resolved_names: | ||
| try: | ||
| from praisonaiagents import tools as tool_module | ||
| func = getattr(tool_module, name, None) |
There was a problem hiding this comment.
The import from praisonaiagents import tools as tool_module is inside a loop. Moving it outside the loop improves performance by avoiding redundant import attempts for each tool in the list.
try:
from praisonaiagents import tools as tool_module
except ImportError:
tool_module = None
for name in resolved_names:
try:
func = getattr(tool_module, name, None) if tool_module else None| environment instead of on the host. | ||
| """ | ||
| import inspect | ||
| import asyncio |
There was a problem hiding this comment.
asyncio is already imported at the top of the file (line 24). This local import is redundant and can be removed.
| import asyncio | |
| import inspect |
There was a problem hiding this comment.
Pull request overview
This PR hardens LocalManagedAgent execution by preventing implicit host-side pip installs and routing certain shell/file tools through an attached compute provider, aligning behavior with the “sandboxed managed agents” safety expectations in #1426.
Changes:
- Add a shared
ManagedSandboxRequiredexception to gate unsafe host package installation. - Make
LocalManagedAgent“sandbox-first” for package installs (compute when attached; otherwise require explicithost_packages_ok=Trueopt-out). - Bridge
execute_command/read_file/write_file/list_filesto compute execution when a compute provider is attached, and add a new test suite covering sandbox/tool bridging behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/praisonai/praisonai/integrations/managed_local.py |
Adds host_packages_ok, enforces sandboxed package install behavior, and introduces compute-bridged tool wrappers for shell/file tools. |
src/praisonai/praisonai/integrations/managed_agents.py |
Introduces ManagedSandboxRequired as a shared exception for sandbox safety enforcement. |
src/praisonai-agents/tests/managed/test_managed_factory.py |
Adds tests for sandbox safety and compute tool bridging behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_install_packages_with_compute_uses_sandbox(self): | ||
| """Test that package installation with compute provider uses sandbox.""" | ||
| from unittest.mock import AsyncMock, patch | ||
| from praisonai.integrations.managed_local import LocalManagedAgent, LocalManagedConfig | ||
|
|
||
| cfg = LocalManagedConfig(packages={"pip": ["requests"]}) | ||
| agent = LocalManagedAgent(config=cfg, compute="local") | ||
|
|
||
| # Mock the compute execution | ||
| with patch.object(agent, 'provision_compute') as mock_provision, \ | ||
| patch.object(agent._compute, 'execute') as mock_execute, \ | ||
| patch('asyncio.run') as mock_asyncio_run, \ | ||
| patch('asyncio.get_event_loop') as mock_get_loop: | ||
|
|
||
| mock_provision.return_value = None | ||
| mock_execute.return_value = {"exit_code": 0, "stdout": "installed"} | ||
| agent._compute_instance_id = "test_instance" | ||
| mock_asyncio_run.return_value = {"exit_code": 0, "stdout": "installed"} | ||
|
|
||
| agent._install_packages() | ||
|
|
||
| # Verify subprocess.run was NOT called (no host installation) | ||
| with patch('praisonai.integrations.managed_local.subprocess.run') as mock_run: | ||
| agent._install_packages() | ||
| mock_run.assert_not_called() | ||
|
|
| if self._compute_instance_id is None: | ||
| try: | ||
| loop = asyncio.get_event_loop() | ||
| loop.run_until_complete(self.provision_compute()) | ||
| except RuntimeError: | ||
| asyncio.run(self.provision_compute()) | ||
|
|
| if tool_name == "execute_command": | ||
| # For execute_command, directly route to compute | ||
| command = args[0] if args else kwargs.get("command", "") | ||
| if not command: | ||
| return "Error: No command specified" | ||
|
|
||
| try: | ||
| try: | ||
| loop = asyncio.get_event_loop() | ||
| result = loop.run_until_complete( | ||
| self._compute.execute(self._compute_instance_id, command) | ||
| ) | ||
| except RuntimeError: | ||
| result = asyncio.run( | ||
| self._compute.execute(self._compute_instance_id, command) | ||
| ) | ||
|
|
||
| # Format result similar to local execute_command | ||
| if result.get("exit_code", 0) == 0: | ||
| return result.get("stdout", "") | ||
| else: | ||
| return f"Command failed (exit {result.get('exit_code', 1)}): {result.get('stderr', '')}" | ||
|
|
||
| except Exception as e: | ||
| return f"Compute execution error: {e}" | ||
|
|
||
| elif tool_name in {"read_file", "write_file", "list_files"}: | ||
| # For file operations, convert to shell commands in compute | ||
| return self._bridge_file_tool(tool_name, *args, **kwargs) | ||
|
|
| command = f'cat "{filepath}"' | ||
|
|
||
| elif tool_name == "write_file": | ||
| filepath = args[0] if args else kwargs.get("filepath", "") | ||
| content = args[1] if len(args) > 1 else kwargs.get("content", "") | ||
| if not filepath: | ||
| return "Error: No filepath specified" | ||
|
|
||
| # Escape content for shell | ||
| import shlex | ||
| command = f'cat > "{filepath}" << "EOF"\n{content}\nEOF' | ||
|
|
||
| elif tool_name == "list_files": | ||
| directory = args[0] if args else kwargs.get("directory", ".") | ||
| command = f'ls -la "{directory}"' | ||
|
|
||
| else: | ||
| return f"Error: Unsupported bridged tool: {tool_name}" | ||
|
|
||
| try: | ||
| try: | ||
| loop = asyncio.get_event_loop() | ||
| result = loop.run_until_complete( | ||
| self._compute.execute(self._compute_instance_id, command) | ||
| ) | ||
| except RuntimeError: | ||
| result = asyncio.run( | ||
| self._compute.execute(self._compute_instance_id, command) | ||
| ) | ||
|
|
||
| if result.get("exit_code", 0) == 0: | ||
| return result.get("stdout", "") | ||
| else: | ||
| return f"Command failed (exit {result.get('exit_code', 1)}): {result.get('stderr', '')}" | ||
|
|
||
| except Exception as e: | ||
| return f"Compute execution error: {e}" |
| if self._compute_instance_id is None: | ||
| try: | ||
| import asyncio | ||
| loop = asyncio.get_event_loop() | ||
| loop.run_until_complete(self.provision_compute()) | ||
| except RuntimeError: | ||
| # No event loop, create one | ||
| asyncio.run(self.provision_compute()) | ||
|
|
||
| pip_cmd = "python -m pip install -q " + " ".join(f'"{pkg}"' for pkg in pip_pkgs) | ||
| logger.info("[local_managed] installing pip packages in compute: %s", pip_pkgs) | ||
|
|
||
| try: | ||
| # Run installation synchronously in compute | ||
| import asyncio | ||
| try: | ||
| loop = asyncio.get_event_loop() | ||
| result = loop.run_until_complete( | ||
| self._compute.execute(self._compute_instance_id, pip_cmd, timeout=120) | ||
| ) | ||
| except RuntimeError: | ||
| # No event loop, create one | ||
| result = asyncio.run( | ||
| self._compute.execute(self._compute_instance_id, pip_cmd, timeout=120) | ||
| ) |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job • Final Architecture Review of PR #1442 ✅Task List:
✅ APPROVED - Security Issues Resolved📋 Architecture Review Summary✅ AGENTS.md Compliance:
🔒 Security Fixes Implemented (Post-Merge)I've implemented comprehensive security fixes addressing all critical vulnerabilities identified by multiple reviewers: 1. Shell Injection Protection (
2. Event Loop Safety (
3. Performance Improvements
4. Test Coverage (
🎯 Validation Results✅ ManagedSandboxRequired exception works correctly
✅ host_packages_ok=True bypass functions properly
✅ shlex.quote() protects against shell injection
✅ Base64 encoding prevents file content injection
✅ _run_async_safe() handles all event loop scenarios📂 Files Modified in Security FixThe following security patches were applied as post-merge fixes:
🎉 Final Verdict: APPROVEDOriginal PR #1442 is architecturally sound and successfully implements the sandbox security requirements. The critical security vulnerabilities have been resolved in the post-merge security patch above. Key Achievements:
Recommendation: Merge the security fix PR above immediately to close all security gaps. |
Focused re-do of the #1426 scope from closed monolithic PR #1437, rebased onto current main.
Scope (3 files)
praisonai/integrations/managed_agents.py— newManagedSandboxRequiredexception.praisonai/integrations/managed_local.py— sandbox-first:_install_packagesroutes to compute when attached; raisesManagedSandboxRequiredotherwise (opt-out viaLocalManagedConfig(host_packages_ok=True)). Tool execution bridged through_create_compute_bridge_toolforexecute_command/read_file/write_file/list_fileswhen a compute provider is attached.tests/managed/test_managed_factory.py— newTestComputeToolBridgesuite (12 tests).Local validation
asyncio.runfallback path — tests originally mocked onlyasyncio.runbut code triesget_event_loop().run_until_completefirst in pytest).Acceptance criteria matched
ManagedSandboxRequiredraised whenpackages=…without compute.LocalManagedConfig(host_packages_ok=True)permits host install.pip installruns inside compute instance.execute_command/read_file/write_file/list_filesbridged through compute.managed-agents.mdx,managed-agents-local.mdx) — deferred; doc-only change, can be addressed in a doc PR without blocking.Closes #1426.
Summary by CodeRabbit
New Features
Security Improvements
host_packages_okconfiguration controls host-level installations.ManagedSandboxRequirederror prevents unsafe package operations.Configuration Changes
host_packages_okflag instead ofsandbox_type.