feat(sandbox): harden local file access and mask host paths#983
feat(sandbox): harden local file access and mask host paths#983WillemJiang wants to merge 7 commits intomainfrom
Conversation
- enforce local sandbox file tools to only accept /mnt/user-data paths - add path traversal checks against thread workspace/uploads/outputs roots - preserve requested virtual paths in tool error messages (no host path leaks) - mask local absolute paths in bash output back to virtual sandbox paths - update bash tool guidance to prefer thread-local venv + python -m pip - add regression tests for path mapping, masking, and access restrictions Fixes #968
- validate absolute path usage in local-mode bash commands - allow only /mnt/user-data virtual paths for user data access - keep a small allowlist for system executable/device paths - return clear permission errors for unsafe command paths - add regression tests for bash path validation rules
There was a problem hiding this comment.
Pull request overview
This PR hardens local file access in the sandbox by:
- Enforcing that local sandbox tools only accept
/mnt/user-datavirtual paths - Adding path traversal protection via
resolve_local_tool_path - Masking host absolute paths in tool output via
mask_local_paths_in_output - Validating bash command paths against an allowlist via
validate_local_bash_command_paths - Updating bash tool guidance to use thread-local venv
- Adding regression tests
Changes:
- Refactored
replace_virtual_pathand added_thread_virtual_to_actual_mappings/_thread_actual_to_virtual_mappingshelpers - Added
mask_local_paths_in_output,resolve_local_tool_path, andvalidate_local_bash_command_pathssecurity functions - Updated all tool functions (
bash_tool,ls_tool,read_file_tool,write_file_tool,str_replace_tool) to use the new security functions and preserve virtual paths in error messages - Added
test_sandbox_tools_security.pywith 5 regression tests
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
backend/src/sandbox/tools.py |
Core changes: new security helpers, path validation, path masking, and updated tool handlers |
backend/tests/test_sandbox_tools_security.py |
New regression tests for path mapping, masking, and access restrictions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except PermissionError: | ||
| return f"Error: Permission denied: {path}" | ||
| return f"Error: Permission denied: {requested_path}" |
There was a problem hiding this comment.
The PermissionError raised by resolve_local_tool_path (e.g., "Only paths under /mnt/user-data are allowed" or "Access denied: path traversal detected") is caught by the bare except PermissionError: blocks in ls_tool, read_file_tool, write_file_tool, and str_replace_tool, which discard the error message and replace it with a generic "Permission denied: {requested_path}" message.
In contrast, bash_tool correctly uses except PermissionError as e: return f"Error: {e}" which preserves the rejection reason. The other tool handlers should do the same so the agent understands why the path was rejected (e.g., that it must use a /mnt/user-data/... path instead).
| except PermissionError: | ||
| return f"Error: Permission denied reading file: {path}" | ||
| return f"Error: Permission denied reading file: {requested_path}" |
There was a problem hiding this comment.
Same issue as in ls_tool: the except PermissionError: block discards the rejection reason from resolve_local_tool_path, replacing it with a generic message. The agent will not know it needs to use a /mnt/user-data/... path. The handler should use except PermissionError as e: return f"Error: {e}" to preserve the message, consistent with how bash_tool handles it.
| except PermissionError: | ||
| return f"Error: Permission denied writing to file: {path}" | ||
| return f"Error: Permission denied writing to file: {requested_path}" |
There was a problem hiding this comment.
Same issue as in ls_tool and read_file_tool: the except PermissionError: block discards the rejection reason from resolve_local_tool_path. The handler should use except PermissionError as e: return f"Error: {e}" to preserve the message, consistent with how bash_tool handles it.
| except PermissionError: | ||
| return f"Error: Permission denied accessing file: {path}" | ||
| return f"Error: Permission denied accessing file: {requested_path}" |
There was a problem hiding this comment.
Same issue as in ls_tool, read_file_tool, and write_file_tool: the except PermissionError: block discards the rejection reason from resolve_local_tool_path. The handler should use except PermissionError as e: return f"Error: {e}" to preserve the message, consistent with how bash_tool handles it.
backend/src/sandbox/tools.py
Outdated
| if path != VIRTUAL_PATH_PREFIX and not path.startswith(f"{VIRTUAL_PATH_PREFIX}/"): | ||
| raise PermissionError(f"Only paths under {VIRTUAL_PATH_PREFIX} are allowed") | ||
|
|
||
| resolved_path = replace_virtual_path(path, thread_data) | ||
| resolved = Path(resolved_path).resolve() | ||
|
|
||
| allowed_roots = [ | ||
| Path(p).resolve() | ||
| for p in ( | ||
| thread_data.get("workspace_path"), | ||
| thread_data.get("uploads_path"), | ||
| thread_data.get("outputs_path"), | ||
| ) | ||
| if p is not None | ||
| ] | ||
|
|
||
| if not allowed_roots: | ||
| raise SandboxRuntimeError("No allowed local sandbox directories configured") | ||
|
|
||
| for root in allowed_roots: | ||
| try: | ||
| resolved.relative_to(root) | ||
| return str(resolved) | ||
| except ValueError: | ||
| continue | ||
|
|
||
| raise PermissionError("Access denied: path traversal detected") |
There was a problem hiding this comment.
The condition at line 128 allows path == VIRTUAL_PATH_PREFIX (i.e., /mnt/user-data) through the initial check, but the subsequent traversal verification at lines 147–154 will always reject it, because replace_virtual_path maps /mnt/user-data to the common parent directory (e.g., /tmp/deer-flow/.../user-data), which is not a subdirectory of any of the allowed_roots (workspace, uploads, outputs).
This means calling ls_tool with /mnt/user-data will pass the first check but then raise a misleading "Access denied: path traversal detected" error, even though there is no traversal attempt. Either the path == VIRTUAL_PATH_PREFIX branch in the condition on line 128 should be removed (to reject the path early with an informative message), or the common parent should be added to allowed_roots if listing /mnt/user-data is intentionally supported.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| def test_resolve_local_tool_path_rejects_path_traversal() -> None: | ||
| base = Path("/tmp/deer-flow/threads/t1/user-data") | ||
| thread_data = { | ||
| "workspace_path": str(base / "workspace"), | ||
| "uploads_path": str(base / "uploads"), | ||
| "outputs_path": str(base / "outputs"), | ||
| } | ||
|
|
||
| with pytest.raises(PermissionError, match="path traversal"): | ||
| resolve_local_tool_path(f"{VIRTUAL_PATH_PREFIX}/workspace/../../../../etc/passwd", thread_data) |
There was a problem hiding this comment.
There is no test for the success path of resolve_local_tool_path: a valid virtual path like /mnt/user-data/workspace/file.txt should be resolved to the corresponding actual host path and returned. Without this test it's possible to introduce a regression in the function that would break the tools' happy path while all existing tests still pass (since they only exercise error cases).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@WillemJiang I've opened a new pull request, #985, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@WillemJiang I've opened a new pull request, #991, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@WillemJiang I've opened a new pull request, #992, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * test(sandbox): add success path test for resolve_local_tool_path Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com>
…lve_local_tool_path (#991) * Initial plan * fix(sandbox): reject bare virtual root early with clear error in resolve_local_tool_path Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: WillemJiang <219644+WillemJiang@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
feat(sandbox): harden local file access and mask host paths
Fixes #968