Skip to content

fix: allow symlinked local sandbox workspace paths#2997

Open
yu-xin-c wants to merge 2 commits into
bytedance:mainfrom
yu-xin-c:fix/local-sandbox-symlink-path-validation
Open

fix: allow symlinked local sandbox workspace paths#2997
yu-xin-c wants to merge 2 commits into
bytedance:mainfrom
yu-xin-c:fix/local-sandbox-symlink-path-validation

Conversation

@yu-xin-c
Copy link
Copy Markdown

Summary

  • allow local sandbox user-data paths to be validated lexically instead of resolving symlinks out of the workspace
  • keep parent-directory traversal blocked by normalizing .. before checking allowed roots
  • add a regression test for a symlinked repository inside /mnt/user-data/workspace

Context

This addresses the path-validation asymmetry discussed in #2820: dedicated tools such as read_file, ls, grep, and glob can reject symlinked workspace repositories after Path.resolve() follows the symlink outside the allowlist, while bash path replacement still works.

Tests

  • uv run --project backend pytest backend/tests/test_sandbox_tools_security.py -q
  • uv run --project backend ruff check backend/packages/harness/deerflow/sandbox/tools.py backend/tests/test_sandbox_tools_security.py
  • python3 -m py_compile backend/packages/harness/deerflow/sandbox/tools.py backend/tests/test_sandbox_tools_security.py

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates local sandbox user-data path validation so symlinked repositories under the workspace are accepted without resolving them outside the allowlist.

Changes:

  • Switches user-data validation from Path.resolve() to lexical abspath() normalization.
  • Documents why symlink traversal is intentionally not resolved.
  • Adds a regression test for a symlinked workspace repository path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
backend/packages/harness/deerflow/sandbox/tools.py Changes local sandbox user-data path normalization/validation behavior.
backend/tests/test_sandbox_tools_security.py Adds symlinked workspace path regression coverage.

"""
resolved_str = replace_virtual_path(path, thread_data)
resolved = Path(resolved_str).resolve()
resolved = Path(os.path.abspath(resolved_str))
@WillemJiang
Copy link
Copy Markdown
Collaborator

@yu-xin-c thanks for your contribution. Please check out the unit test error and review the comment of Copilot.

@yu-xin-c yu-xin-c force-pushed the fix/local-sandbox-symlink-path-validation branch from 024cde8 to 358a187 Compare May 17, 2026 04:47
@WillemJiang
Copy link
Copy Markdown
Collaborator

@yu-xin-c, here are some comments for the latest code

  1. Add a traversal-through-symlink test:
def test_workspace_symlink_does_not_allow_dotdot_escape(tmp_path):
    # workspace/repo -> /tmp/external
    # request /mnt/user-data/workspace/repo/../../etc/passwd
    # should still be blocked
  1. Add a one-line comment on thelexical_path = Path(os.path.abspath(...))line explaining that os.path.abspath normalizes .. without following symlinks, which is the intended behavior for workspace paths.
  2. Consider renaming _is_workspace_user_data_path to something like _is_virtual_workspace_path to make it clearer it operates on virtual paths, not resolved host paths.

@yu-xin-c
Copy link
Copy Markdown
Author

Updated in 76048ce:\n\n- Added a regression test for a workspace symlink followed by a .. traversal escape.\n- Added a short comment explaining the lexical abspath check.\n- Renamed _is_workspace_user_data_path to _is_virtual_workspace_path.\n\nLocal verification:\n- uv run ruff format packages/harness/deerflow/sandbox/tools.py tests/test_sandbox_tools_security.py\n- uv run ruff check packages/harness/deerflow/sandbox/tools.py tests/test_sandbox_tools_security.py\n- uv run pytest tests/test_sandbox_tools_security.py -q -> 102 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants