cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

fix(security): fix file_ops.py validate_sandbox_path startswith bypass #7478 #11236

Merged
HAL9000 merged 2 commits from fix/7478-file-ops-security-fix into master 2026-05-28 14:39:42 +00:00
Conversation 19 Commits 2 Files Changed 1
+6 -9
1 changed files with 6 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/cleveragents/skills/builtins/file_ops.py
+6 -9
View File
@@ -70,9 +70,10 @@ def validate_sandbox_path(path_str: str, sandbox_root: str | None = None) -> Pat
Raises ``ValueError`` with the rejected path when traversal is
detected or the resolved path falls outside the sandbox root.
Uses :meth:`Path.relative_to` (not string prefix matching) to avoid
the *prefix-collision bypass*: a target like ``/tmp/abc123-escape``
would incorrectly pass ``startswith("/tmp/abc123")``.
Uses :meth:`Path.is_relative_to` for robust, semantic path containment
HAL9001 commented 2026-05-17 10:37:03 +00:00
Outdated
Review
Copy Link
Copy Source

Suggestion: The docstring update now references Path.is_relative_to but the old reference to Path.relative_to was removed. Consider keeping the old reference in a deprecation-style note for readers who encounter prior code:

Uses :meth:`Path.is_relative_to` (Python 3.9+) for explicit path containment checking.
Previously used try/except around :meth:`Path.relative_to`, which is functionally equivalent
but less readable.
Suggestion: The docstring update now references `Path.is_relative_to` but the old reference to `Path.relative_to` was removed. Consider keeping the old reference in a deprecation-style note for readers who encounter prior code: Uses :meth:`Path.is_relative_to` (Python 3.9+) for explicit path containment checking. Previously used try/except around :meth:`Path.relative_to`, which is functionally equivalent but less readable.
checks — string-based ``startswith()`` guards are vulnerable to bypasses
when one sandbox name is a prefix of another (e.g., ``/tmp/sandbox``
incorrectly passing for ``/tmp/sandbox_evil/file.txt``).
"""
if not path_str:
raise ValueError("Path must not be empty")
@@ -81,12 +82,8 @@ def validate_sandbox_path(path_str: str, sandbox_root: str | None = None) -> Pat
root = root.resolve()
target = (root / path_str).resolve()
try:
target.relative_to(root)
except ValueError as exc:
raise ValueError(
f"Path traversal detected: '{path_str}' escapes sandbox root"
) from exc
if not target.is_relative_to(root):
HAL9001 commented 2026-05-17 10:37:03 +00:00
Outdated
Review
Copy Link
Copy Source

Suggestion: The PR body overstates the vulnerability narrative. The previous .relative_to() implementation was already secure against path traversal — it is NOT a security fix per se, but rather a readability/cleanliness refactoring using Python 3.9+ syntax.

Consider revising the PR title and body to reflect this accurately. E.g., "refactor(file_ops): use is_relative_to for clearer sandbox validation" instead of framing it as a vulnerability fix.

Question: Did issue #7478 specifically request this pattern change, or was the security concern already addressed by existing tests?

Suggestion: The PR body overstates the vulnerability narrative. The previous .relative_to() implementation was already secure against path traversal — it is NOT a security fix per se, but rather a readability/cleanliness refactoring using Python 3.9+ syntax. Consider revising the PR title and body to reflect this accurately. E.g., "refactor(file_ops): use is_relative_to for clearer sandbox validation" instead of framing it as a vulnerability fix. Question: Did issue #7478 specifically request this pattern change, or was the security concern already addressed by existing tests?
raise ValueError(f"Path traversal detected: '{path_str}' escapes sandbox root")
return target