Skip to content

[Security] Address high-severity command injection risk in edit_tool directory view#393

Open
13ernkastel wants to merge 2 commits into
bytedance:mainfrom
13ernkastel:fix-edit-tool-view-shell-injection
Open

[Security] Address high-severity command injection risk in edit_tool directory view#393
13ernkastel wants to merge 2 commits into
bytedance:mainfrom
13ernkastel:fix-edit-tool-view-shell-injection

Conversation

@13ernkastel
Copy link
Copy Markdown

@13ernkastel 13ernkastel commented Mar 29, 2026

Security Context

This PR addresses a high-severity command-injection risk in str_replace_based_edit_tool directory views.

CVSS

CVSS v3.1: 7.8 (AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H)
This is scored as high rather than critical because exploitation requires user interaction and control over a local path input, not a remotely reachable unauthenticated attack surface.

Classification

  • CWE-78: OS Command Injection

Affected paths

  • trae_agent/tools/edit_tool.py
  • trae_agent/tools/edit_tool_cli.py
  • evaluation/patch_selection/trae_selector/tools/tools/edit.py

Summary

  • replace shell-based directory listing in str_replace_based_edit_tool with a pure-Python os.scandir() walker
  • apply the same fix to the packaged CLI copy and the evaluation harness copy
  • add regression tests covering hidden-file filtering, shell-metacharacter paths, depth limits, and symlinked directories

Root cause

When view was used on a directory, the tool constructed a shell command using the supplied path and executed it via a shell helper. Because the path was not treated as inert data, crafted directory names containing shell metacharacters could trigger unintended host-side command execution.

Why this is affected

The vulnerable flow is straightforward:

  1. str_replace_based_edit_tool accepts a caller-supplied path argument for view.
  2. validate_path() only checks that the path is absolute, exists, and is a directory when view is used.
  3. _view() then interpolated that path directly into a command string of the form find {path} -maxdepth 2 -not -path '*/\\.*'.
  4. That command string was executed through the shell helper, which uses asyncio.create_subprocess_shell(...).
  5. Because the path was embedded into a shell command without quoting or escaping, any shell metacharacters present in the directory name were interpreted by the shell instead of being treated as literal path characters.

This means the bug is not in path validation itself, but in the trust boundary after validation: a valid existing directory path was later reinterpreted as shell syntax.

This is especially risky because:

  • directory names on Unix-like systems can legally contain shell metacharacters such as ;, &, |, backticks, and $()
  • the affected operation is a benign-looking view request rather than an explicitly dangerous execution tool
  • the same pattern existed in the main tool implementation, the packaged CLI copy, and the evaluation harness copy

Impact

Under the right conditions, this could allow commands to run as the same user executing trae-cli. In practice, that means access to files, credentials, and repository contents available to that user, along with the ability to modify or damage the local workspace.

Scope

This PR fixes the directory view injection path in the edit tool family. It does not attempt to change broader execution surfaces such as the explicit bash tool or other unrelated shell-construction sites.

Fix

Use a pure-Python directory walker that:

  • never invokes a shell
  • preserves the existing "list up to 2 levels deep" behavior
  • continues excluding hidden entries
  • avoids descending into symlinked directories by using is_dir(follow_symlinks=False)

This removes the injection surface rather than trying to escape around it.

Why os.scandir()

os.scandir() was chosen intentionally here because it gives a direct filesystem enumeration API instead of reconstructing the old behavior through a shell command. That matters for both security and correctness:

  • it treats the directory path as data rather than shell syntax, so metacharacters in path names are never interpreted as commands
  • it exposes DirEntry metadata directly, which makes it easy to sort entries and check is_dir(follow_symlinks=False) without extra shell parsing
  • it preserves the existing listing behavior while making symlink traversal decisions explicit in code

In short, os.scandir() is not just a refactor convenience here; it is part of the security fix because it removes the shell from the directory-view path entirely.

Compatibility

The intended user-visible behavior is unchanged: directory view still returns non-hidden entries up to two levels deep. The implementation is now deterministic and shell-free, and symlinked directories are listed but no longer traversed as directories.

Validation

  • uv run --extra test --extra evaluation pytest tests/tools/test_edit_tool.py
  • uv run --extra test --extra evaluation pytest tests/tools/test_json_edit_tool.py
  • uv run ruff check trae_agent/tools/edit_tool.py trae_agent/tools/edit_tool_cli.py evaluation/patch_selection/trae_selector/tools/tools/edit.py tests/tools/test_edit_tool.py

@13ernkastel 13ernkastel marked this pull request as ready for review March 29, 2026 07:54
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.

1 participant