feat: code sandbox - #291
Conversation
Reviewer's GuideIntroduce a generalized sandboxed code execution subsystem with Docker and subprocess backends, shared capture/state utilities, and Corral Tool wrappers, while refactoring existing output-capture logic out of code_tools into reusable helpers and covering the new functionality with tests. Sequence diagram for executing Python code in Docker sandboxsequenceDiagram
actor User
participant SandboxCodeTool
participant DockerSandbox as DockerSandbox
participant Capture as CaptureUtils
participant State as StateUtils
participant Docker as DockerDaemon
User->>SandboxCodeTool: execute(python_code, timeout)
SandboxCodeTool->>DockerSandbox: execute(code, timeout)
DockerSandbox->>DockerSandbox: _ensure_started()
alt persistent_state enabled
DockerSandbox->>State: generate_state_restore_code(state_file)
State-->>DockerSandbox: restore_code
end
DockerSandbox->>Capture: generate_output_capture_code()
Capture-->>DockerSandbox: capture_code
alt persistent_state enabled
DockerSandbox->>State: generate_state_save_code(state_file)
State-->>DockerSandbox: save_code
end
DockerSandbox->>DockerSandbox: build full_code
DockerSandbox->>DockerDaemon: docker cp local_script -> container
DockerDaemon-->>DockerSandbox: cp result
DockerSandbox->>DockerDaemon: docker exec python container_script
DockerDaemon-->>DockerSandbox: stdout, stderr, return_code
DockerSandbox->>Capture: parse_execution_output(stdout)
Capture-->>DockerSandbox: execution_result, output_lines
DockerSandbox->>DockerSandbox: truncate stdout if needed
DockerSandbox-->>SandboxCodeTool: ExecResult
SandboxCodeTool->>SandboxCodeTool: to_tool_result()
SandboxCodeTool-->>User: JSON tool result
Class diagram for sandbox core types and backendsclassDiagram
class Sandbox {
<<abstract>>
- SandboxConfig config
- bool _started
+ start() void
+ stop() void
+ execute(code: str, timeout: int, env: dict[str,str]) ExecResult
+ execute_command(command: str, timeout: int, cwd: str) ExecResult
+ upload_file(local_path: str|Path, sandbox_path: str) void
+ download_file(sandbox_path: str, local_path: str|Path) void
+ __enter__() Sandbox
+ __exit__(exc_type, exc_val, exc_tb) bool
}
class DockerSandbox {
- str _container_name
- str _container_id
- str _image_tag
+ __init__(config: SandboxConfig)
+ start() void
+ stop() void
+ execute(code: str, timeout: int, env: dict[str,str]) ExecResult
+ execute_command(command: str, timeout: int, cwd: str) ExecResult
+ upload_file(local_path: str|Path, sandbox_path: str) void
+ download_file(sandbox_path: str, local_path: str|Path) void
- _ensure_started() void
- _resolve_image() str
- _install_packages() void
- _disable_network() void
}
class SubprocessSandbox {
- Path _work_dir
- Path _state_file
+ __init__(config: SandboxConfig)
+ start() void
+ stop() void
+ execute(code: str, timeout: int, env: dict[str,str]) ExecResult
+ execute_command(command: str, timeout: int, cwd: str) ExecResult
+ upload_file(local_path: str|Path, sandbox_path: str) void
+ download_file(sandbox_path: str, local_path: str|Path) void
- _ensure_started() void
}
class SandboxConfig {
+ str backend
+ str docker_image
+ str dockerfile
+ str working_dir
+ list[str] python_packages
+ int pip_install_timeout
+ ResourceLimits resources
+ NetworkPolicy network
+ bool persistent_state
+ str state_dir
}
class ResourceLimits {
+ int cpu_count
+ int memory_mb
+ int storage_mb
+ int timeout_seconds
+ int max_output_bytes
}
class NetworkPolicy {
+ bool allow_network
+ list[str] allowed_hosts
}
class ExecResult {
+ bool success
+ str stdout
+ str stderr
+ int return_code
+ dict[str,Any] execution_result
+ str error
+ bool timed_out
+ float duration_seconds
+ to_tool_result() str
}
class CaptureUtils {
<<utility>>
+ generate_output_capture_code() str
+ parse_execution_output(stdout: str) (dict, list[str])
}
class StateUtils {
<<utility>>
+ generate_state_restore_code(state_file: str) str
+ generate_state_save_code(state_file: str) str
}
Sandbox <|-- DockerSandbox
Sandbox <|-- SubprocessSandbox
SandboxConfig --> ResourceLimits
SandboxConfig --> NetworkPolicy
Sandbox --> SandboxConfig
DockerSandbox --> ExecResult
SubprocessSandbox --> ExecResult
DockerSandbox ..> CaptureUtils
DockerSandbox ..> StateUtils
SubprocessSandbox ..> CaptureUtils
SubprocessSandbox ..> StateUtils
CaptureUtils ..> ExecResult
Class diagram for sandbox tool wrappersclassDiagram
class Tool {
<<external>>
}
class ToolArgument {
<<external>>
+ str name
+ str type
+ str description
+ bool required
+ Any default
}
class Sandbox {
<<abstract>>
}
class SandboxCodeTool {
- Sandbox _sandbox
+ __init__(sandbox: Sandbox)
+ execute(python_code: str, timeout: int) str
}
class SandboxTerminalTool {
- Sandbox _sandbox
+ __init__(sandbox: Sandbox)
+ execute(command: str, timeout: int) str
}
class SandboxToolModule {
<<module>>
+ create_sandbox(config: SandboxConfig) Sandbox
+ create_sandbox_tools(config: SandboxConfig) (Sandbox, dict[str,Tool])
}
class SandboxConfig {
}
Tool <|-- SandboxCodeTool
Tool <|-- SandboxTerminalTool
SandboxCodeTool --> Sandbox
SandboxTerminalTool --> Sandbox
SandboxToolModule ..> SandboxConfig
SandboxToolModule ..> Sandbox
SandboxToolModule ..> SandboxCodeTool
SandboxToolModule ..> SandboxTerminalTool
SandboxCodeTool ..> ToolArgument
SandboxTerminalTool ..> ToolArgument
Flow diagram for sandbox creation via factory functionsflowchart TD
Start["Caller code"]
CFG["SandboxConfig (optional)"]
Factory["create_sandbox(config)"]
BackendCheck["Check config.backend"]
DockerSB["DockerSandbox(config)"]
SubprocSB["SubprocessSandbox(config)"]
SBOut["Sandbox instance"]
Start --> CFG
CFG --> Factory
Factory --> BackendCheck
BackendCheck -->|backend == 'docker'| DockerSB
BackendCheck -->|backend == 'subprocess'| SubprocSB
DockerSB --> SBOut
SubprocSB --> SBOut
subgraph ToolsFactory
FactoryTools["create_sandbox_tools(config)"]
MakeCodeTool["SandboxCodeTool(sandbox)"]
MakeTermTool["SandboxTerminalTool(sandbox)"]
ToolsOut["{ 'execute_python_code': ..., 'run_in_terminal': ... }"]
end
SBOut --> FactoryTools
FactoryTools --> MakeCodeTool
FactoryTools --> MakeTermTool
MakeCodeTool --> ToolsOut
MakeTermTool --> ToolsOut
ToolsOut --> Start
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Hey - I've found 6 security issues, 2 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead. (link)
General comments:
- In
DockerSandbox._disable_network, theNetworkPolicy.allowed_hostsfield is currently unused and the implementation just disconnects the container from all networks; consider either enforcing host-level allow rules or documenting that only a full on/off network toggle is supported right now. SubprocessSandbox.execute_commandusesshell=Trueand passes the command string directly; if this code can ever receive untrusted input, it would be safer to avoidshell=Trueor at least clearly constrain/document the intended trust model for commands.- The Docker backend builds images (
_resolve_image) and stores the tag but never cleans the image up instop(), which could lead to image accumulation over time; consider adding optional image cleanup or a reuse strategy for identical Dockerfiles.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DockerSandbox._disable_network`, the `NetworkPolicy.allowed_hosts` field is currently unused and the implementation just disconnects the container from all networks; consider either enforcing host-level allow rules or documenting that only a full on/off network toggle is supported right now.
- `SubprocessSandbox.execute_command` uses `shell=True` and passes the command string directly; if this code can ever receive untrusted input, it would be safer to avoid `shell=True` or at least clearly constrain/document the intended trust model for commands.
- The Docker backend builds images (`_resolve_image`) and stores the tag but never cleans the image up in `stop()`, which could lead to image accumulation over time; consider adding optional image cleanup or a reuse strategy for identical Dockerfiles.
## Individual Comments
### Comment 1
<location> `src/corral/sandbox/config.py:11` </location>
<code_context>
+ memory_mb: int = 2048 # TODO: ml task might require more
+ storage_mb: int = 4096
+ timeout_seconds: int = 300
+ max_output_bytes: int = 60 * 1024 # needs to be defined in the code_tools.py file
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Max output is described as bytes but enforced on `len(stdout)` (characters), which can be inaccurate for multibyte encodings.
This affects both `DockerSandbox.execute/execute_command` and `SubprocessSandbox.execute/execute_command`, which compare `len(stdout)` to `max_output_bytes`. If the limit is meant to be in bytes, consider enforcing it on encoded output (e.g., `stdout.encode(...)`) or switching to binary mode and using `proc.stdout_bytes`, then truncating at the byte level. Also, the inline comment referencing `code_tools.py` seems stale now that this limit lives on `SandboxConfig`.
</issue_to_address>
### Comment 2
<location> `src/corral/sandbox/_state.py:33-46` </location>
<code_context>
+ __save__[__k__] = __v__
+ except Exception:
+ pass
+with open({state_file!r}, 'wb') as __f__:
+ __pkl__.dump(__save__, __f__)
+del __pkl__, __types__, __save__, __excluded__, __k__, __v__, __f__
</code_context>
<issue_to_address>
**suggestion:** State save logic assumes the parent directory exists, which may fail for custom `state_file` paths.
`open({state_file!r}, 'wb')` will raise if the parent directory doesn’t exist. While built-in backends may create this directory, custom `SandboxConfig.state_dir` values or future backends might not. Consider generating code that first runs `Path(state_file).parent.mkdir(parents=True, exist_ok=True)` before opening the file to avoid these failures.
```suggestion
for __k__, __v__ in dict(locals()).items():
if (not __k__.startswith('__') and
__k__ not in __excluded__ and
not isinstance(__v__, __types__.ModuleType) and
not callable(__v__)):
try:
__pkl__.dumps(__v__)
__save__[__k__] = __v__
except Exception:
pass
from pathlib import Path as __Path
__state_path__ = __Path({state_file!r})
__state_path__.parent.mkdir(parents=True, exist_ok=True)
with __state_path__.open('wb') as __f__:
__pkl__.dump(__save__, __f__)
del __pkl__, __types__, __save__, __excluded__, __k__, __v__, __f__, __Path, __state_path__
"""
```
</issue_to_address>
### Comment 3
<location> `src/corral/sandbox/docker_sandbox.py:66` </location>
<code_context>
result = subprocess.run(cmd, capture_output=True, text=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location> `src/corral/sandbox/docker_sandbox.py:151-157` </location>
<code_context>
proc = subprocess.run(
docker_cmd,
capture_output=True,
text=True,
timeout=timeout,
check=False,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `src/corral/sandbox/docker_sandbox.py:212-218` </location>
<code_context>
proc = subprocess.run(
docker_cmd,
capture_output=True,
text=True,
timeout=timeout,
check=False,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `src/corral/sandbox/subprocess_sandbox.py:98-106` </location>
<code_context>
proc = subprocess.run(
[sys.executable, str(script_path)],
capture_output=True,
text=True,
timeout=timeout,
cwd=str(self._work_dir),
env=proc_env,
check=False,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `src/corral/sandbox/subprocess_sandbox.py:154-162` </location>
<code_context>
proc = subprocess.run(
command,
shell=True,
capture_output=True,
text=True,
timeout=timeout,
cwd=work_cwd,
check=False,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `src/corral/sandbox/subprocess_sandbox.py:156` </location>
<code_context>
shell=True,
</code_context>
<issue_to_address>
**security (python.lang.security.audit.subprocess-shell-true):** Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
```suggestion
shell=False,
```
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| memory_mb: int = 2048 # TODO: ml task might require more | ||
| storage_mb: int = 4096 | ||
| timeout_seconds: int = 300 | ||
| max_output_bytes: int = 60 * 1024 # needs to be defined in the code_tools.py file |
There was a problem hiding this comment.
issue (bug_risk): Max output is described as bytes but enforced on len(stdout) (characters), which can be inaccurate for multibyte encodings.
This affects both DockerSandbox.execute/execute_command and SubprocessSandbox.execute/execute_command, which compare len(stdout) to max_output_bytes. If the limit is meant to be in bytes, consider enforcing it on encoded output (e.g., stdout.encode(...)) or switching to binary mode and using proc.stdout_bytes, then truncating at the byte level. Also, the inline comment referencing code_tools.py seems stale now that this limit lives on SandboxConfig.
| for __k__, __v__ in dict(locals()).items(): | ||
| if (not __k__.startswith('__') and | ||
| __k__ not in __excluded__ and | ||
| not isinstance(__v__, __types__.ModuleType) and | ||
| not callable(__v__)): | ||
| try: | ||
| __pkl__.dumps(__v__) | ||
| __save__[__k__] = __v__ | ||
| except Exception: | ||
| pass | ||
| with open({state_file!r}, 'wb') as __f__: | ||
| __pkl__.dump(__save__, __f__) | ||
| del __pkl__, __types__, __save__, __excluded__, __k__, __v__, __f__ | ||
| """ |
There was a problem hiding this comment.
suggestion: State save logic assumes the parent directory exists, which may fail for custom state_file paths.
open({state_file!r}, 'wb') will raise if the parent directory doesn’t exist. While built-in backends may create this directory, custom SandboxConfig.state_dir values or future backends might not. Consider generating code that first runs Path(state_file).parent.mkdir(parents=True, exist_ok=True) before opening the file to avoid these failures.
| for __k__, __v__ in dict(locals()).items(): | |
| if (not __k__.startswith('__') and | |
| __k__ not in __excluded__ and | |
| not isinstance(__v__, __types__.ModuleType) and | |
| not callable(__v__)): | |
| try: | |
| __pkl__.dumps(__v__) | |
| __save__[__k__] = __v__ | |
| except Exception: | |
| pass | |
| with open({state_file!r}, 'wb') as __f__: | |
| __pkl__.dump(__save__, __f__) | |
| del __pkl__, __types__, __save__, __excluded__, __k__, __v__, __f__ | |
| """ | |
| for __k__, __v__ in dict(locals()).items(): | |
| if (not __k__.startswith('__') and | |
| __k__ not in __excluded__ and | |
| not isinstance(__v__, __types__.ModuleType) and | |
| not callable(__v__)): | |
| try: | |
| __pkl__.dumps(__v__) | |
| __save__[__k__] = __v__ | |
| except Exception: | |
| pass | |
| from pathlib import Path as __Path | |
| __state_path__ = __Path({state_file!r}) | |
| __state_path__.parent.mkdir(parents=True, exist_ok=True) | |
| with __state_path__.open('wb') as __f__: | |
| __pkl__.dump(__save__, __f__) | |
| del __pkl__, __types__, __save__, __excluded__, __k__, __v__, __f__, __Path, __state_path__ | |
| """ |
| # We'll disable it after setup if needed | ||
| cmd.extend([image, "sleep", "infinity"]) | ||
|
|
||
| result = subprocess.run(cmd, capture_output=True, text=True, check=False) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| proc = subprocess.run( | ||
| docker_cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| proc = subprocess.run( | ||
| docker_cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| proc = subprocess.run( | ||
| [sys.executable, str(script_path)], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| cwd=str(self._work_dir), | ||
| env=proc_env, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| proc = subprocess.run( | ||
| command, | ||
| shell=True, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout, | ||
| cwd=work_cwd, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| try: | ||
| proc = subprocess.run( | ||
| command, | ||
| shell=True, |
There was a problem hiding this comment.
security (python.lang.security.audit.subprocess-shell-true): Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead.
| shell=True, | |
| shell=False, |
Source: opengrep
|
Did you consider using an external dependency for that? What added benefit would this bring if we make corral to default to docker use? |
Added a code execution tool sandboxed with docker.
Summary by Sourcery
Introduce a configurable sandboxed code execution framework with Docker and subprocess backends, and expose it via Corral tools for Python code and shell command execution.
New Features:
Enhancements:
Tests: