[Security] Address high-severity command injection risk in edit_tool directory view#393
Open
13ernkastel wants to merge 2 commits into
Open
[Security] Address high-severity command injection risk in edit_tool directory view#39313ernkastel wants to merge 2 commits into
13ernkastel wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Context
This PR addresses a high-severity command-injection risk in
str_replace_based_edit_tooldirectory 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 InjectionAffected paths
trae_agent/tools/edit_tool.pytrae_agent/tools/edit_tool_cli.pyevaluation/patch_selection/trae_selector/tools/tools/edit.pySummary
str_replace_based_edit_toolwith a pure-Pythonos.scandir()walkerRoot cause
When
viewwas 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:
str_replace_based_edit_toolaccepts a caller-suppliedpathargument forview.validate_path()only checks that the path is absolute, exists, and is a directory whenviewis used._view()then interpolated that path directly into a command string of the formfind {path} -maxdepth 2 -not -path '*/\\.*'.asyncio.create_subprocess_shell(...).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:
;,&,|, backticks, and$()viewrequest rather than an explicitly dangerous execution toolImpact
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
viewinjection path in the edit tool family. It does not attempt to change broader execution surfaces such as the explicitbashtool or other unrelated shell-construction sites.Fix
Use a pure-Python directory walker that:
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:DirEntrymetadata directly, which makes it easy to sort entries and checkis_dir(follow_symlinks=False)without extra shell parsingIn 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
viewstill 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.pyuv run --extra test --extra evaluation pytest tests/tools/test_json_edit_tool.pyuv 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