mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-07-28 10:09:21 +00:00
2e95bfb4e8
* fix(tools): sandbox FileWriterTool writes and fix file tool rough edges FileReadTool confined reads to the working directory, but FileWriterTool only checked that `filename` stayed inside `directory` — and `directory` itself is an LLM-supplied schema field. An agent could therefore write anywhere the process had permission to, including ~/.ssh and site-packages, while the reader refused to read back what the writer had just written. FileWriterTool was the only filesystem tool in the package that did not go through validate_file_path; files_compressor_tool validates even its output path. Writes are now confined to base_dir (the working directory by default): the resolved directory must sit inside base_dir, and the resolved file must sit inside that directory. The pre-existing filename containment check is kept as-is and still applies even when the unsafe-paths escape hatch is on, so no existing guarantee is weakened. Both tools gain a base_dir field so a developer can widen the sandbox deliberately instead of reaching for the process-wide CREWAI_TOOLS_ALLOW_UNSAFE_PATHS kill switch. FileReadTool also stops rejecting a file_path given to its own constructor: that is developer-declared intent, and declaring one file does not expose its siblings. Also fixed: - FileReadTool scanned the whole file when reading a line window; it now stops via islice once the requested lines are collected. - FileWriterTool._run(**kwargs) made the documented positional call signature raise TypeError and turned a missing overwrite into "error accessing key". It now takes named parameters in the documented (filename, content, directory) order. - A directory naming an existing file reported "already exists and overwrite option was not passed" even with overwrite=True; it now explains the real problem. - Subdirectories inside filename are created, matching what passing directory already did. - Both tools now write and decode UTF-8 by default instead of the platform locale encoding, with an encoding field to override. The docs already claimed UTF-8 and recommended the writer to Windows users. - The writer's schema fields had no descriptions for the LLM. - Docs claimed FileReadTool parses JSON into a dict (it never has), shipped a snippet that raised TypeError, and did not mention the path sandbox. The writer README also began with a stray "Here's the rewritten README" preamble. BREAKING CHANGE: FileWriterTool no longer writes outside the working directory. Pass base_dir to authorize a different tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: update tool specifications * fix(tools): make the declared FileReadTool file reachable by agents Addresses review feedback on #6692. The constructor-path exemption did not actually work the way an agent calls the tool. The description only advertises a redacted label (the basename, when the file sits outside the sandbox), but resolution required the exact absolute path, so the model's call was sandboxed and the declared file was never read. Worse, file_path was a required schema field, so the long-documented "call with no arguments to read the default file" raised a validation error instead: FileReadTool(file_path="/outside/declared.txt") .run() -> ValueError: validation failed .run(file_path="declared.txt") -> Error: File not found .run(file_path="/outside/declared.txt") -> works, but the model was never told this path file_path is now optional in the schema, so omitting it reads the default, and the declared file is addressable by the label the description shows the model as well as by its real path. Declaring one file still does not expose its siblings. The declared path is also pinned to its real path at construction, so a later chdir cannot silently repoint it at a different file — previously a relative constructor path re-resolved against the new working directory on every call. Also guards the writer's filepath resolution, which could raise ValueError out of _run for a filename containing a null byte, breaking the contract of always returning a descriptive string. The directory and read paths were already guarded. Adds docstrings to strtobool and both _run methods, corrects an Arabic tanween spelling and a kaf-as-descriptor calque in the localized read docs, and regenerates tool.specs.json for the schema change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tools): anchor the declared read path to base_dir, not the cwd Addresses the second round of review feedback on #6692. The previous commit pinned a relative constructor file_path with os.path.realpath, which anchors to the working directory, while both format_path_for_display and validate_file_path anchor a relative path to base_dir. With the two roots disagreeing, the same relative string meant two different files — and the tool served the cwd one under a label that looks like it belongs to the sandbox: FileReadTool(file_path="data.txt", base_dir="/allowed") # cwd=/work label advertised to the model -> "data.txt" run(file_path="data.txt") -> contents of /work/data.txt That reads a file from outside base_dir, so it was a sandbox escape introduced by the exemption itself, not just a wrong-file bug. Resolution now goes through a single _resolve_against_base helper that anchors relative paths exactly the way the sandbox does, so the pinned path, the advertised label and the containment check all agree. Covered by test_relative_declared_path_anchors_to_base_dir. Also softens "always readable" to "always allowed past the containment check" in the docstring, README and docs, since bypassing containment does not guarantee the read succeeds — it can still fail on a missing file, a directory, or permissions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tools): tell the LLM about the path sandbox in tool descriptions Addresses the low-confidence notes from the Copilot review on #6692. Both tools' descriptions were pre-sandbox wording, so the model learned about containment only by attempting a path and reading the error back. Both now state that access is confined to the tool's allowed directory and that a path resolving outside it is rejected. The wording deliberately says "the tool's allowed directory" rather than "the working directory", because the root is base_dir when one is set, and naming the absolute root would leak it into the prompt — the same reason paths are redacted in errors. Not changed: the notes also suggested advertising `encoding`. That is a constructor-only field the model cannot set, so describing it to the LLM would be misleading. Also fixes a test docstring that contradicted its own assertion — the public run() path does raise on schema validation failure, which is what the test asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tools): anchor base_dir at construction so the sandbox cannot move Addresses the third review round on #6692. Both remaining findings came from the same habit: storing an unanchored string and re-resolving it later. A relative base_dir was kept verbatim and re-resolved against getcwd() on every call, while the declared file was pinned once at construction. After a chdir the sandbox root moved but the declared default did not, so one tool applied two different roots. base_dir is now resolved once — in the reader's __init__, and via a field_validator on the writer so it also applies on the model_validate path. That also covers the serialization concern. model_dump drops the private pin, and __init__ re-runs on restore, so a relative file_path was re-anchored against whatever the working directory happened to be at load time. With base_dir anchored, restore rebuilds the identical pin. The residual case is a relative file_path with no base_dir, where the sandbox root is the working directory too — so both move together and the tool stays self-consistent. Covered by test_declared_path_survives_a_serialization_round_trip and test_relative_base_dir_is_anchored_at_construction on both tools. Also corrects the writer's 'directory' description, README and docs: the default resolves inside the tool's allowed directory, which is base_dir when one is set, not always the working directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
212 lines
8.0 KiB
Python
212 lines
8.0 KiB
Python
"""Tests for path and URL validation utilities."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
|
|
import pytest
|
|
|
|
from crewai_tools.security.safe_path import (
|
|
format_path_for_display,
|
|
format_sandbox_error,
|
|
validate_directory_path,
|
|
validate_file_path,
|
|
validate_url,
|
|
)
|
|
|
|
|
|
class TestValidateFilePath:
|
|
"""Tests for validate_file_path."""
|
|
|
|
def test_valid_relative_path(self, tmp_path):
|
|
"""Normal relative path within the base directory."""
|
|
(tmp_path / "data.json").touch()
|
|
result = validate_file_path("data.json", str(tmp_path))
|
|
assert result == str(tmp_path / "data.json")
|
|
|
|
def test_valid_nested_path(self, tmp_path):
|
|
"""Nested path within base directory."""
|
|
(tmp_path / "sub").mkdir()
|
|
(tmp_path / "sub" / "file.txt").touch()
|
|
result = validate_file_path("sub/file.txt", str(tmp_path))
|
|
assert result == str(tmp_path / "sub" / "file.txt")
|
|
|
|
def test_rejects_dotdot_traversal(self, tmp_path):
|
|
"""Reject ../ traversal that escapes base_dir."""
|
|
with pytest.raises(ValueError, match="outside the allowed directory"):
|
|
validate_file_path("../../etc/passwd", str(tmp_path))
|
|
|
|
def test_rejects_absolute_path_outside_base(self, tmp_path):
|
|
"""Reject absolute path outside base_dir."""
|
|
with pytest.raises(ValueError, match="outside the allowed directory"):
|
|
validate_file_path("/etc/passwd", str(tmp_path))
|
|
|
|
def test_allows_absolute_path_inside_base(self, tmp_path):
|
|
"""Allow absolute path that's inside base_dir."""
|
|
(tmp_path / "ok.txt").touch()
|
|
result = validate_file_path(str(tmp_path / "ok.txt"), str(tmp_path))
|
|
assert result == str(tmp_path / "ok.txt")
|
|
|
|
def test_rejects_symlink_escape(self, tmp_path):
|
|
"""Reject symlinks that point outside base_dir."""
|
|
link = tmp_path / "sneaky_link"
|
|
os.symlink("/etc/passwd", str(link))
|
|
with pytest.raises(ValueError, match="outside the allowed directory"):
|
|
validate_file_path("sneaky_link", str(tmp_path))
|
|
|
|
def test_defaults_to_cwd(self):
|
|
"""When no base_dir is given, use cwd."""
|
|
cwd = os.getcwd()
|
|
# A file in cwd should be valid
|
|
result = validate_file_path(".", None)
|
|
assert result == os.path.realpath(cwd)
|
|
|
|
def test_escape_hatch(self, tmp_path, monkeypatch):
|
|
"""CREWAI_TOOLS_ALLOW_UNSAFE_PATHS=true bypasses validation."""
|
|
monkeypatch.setenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", "true")
|
|
# This would normally be rejected
|
|
result = validate_file_path("/etc/passwd", str(tmp_path))
|
|
assert result == os.path.realpath("/etc/passwd")
|
|
|
|
def test_rejection_message_redacts_absolute_prefixes(self, tmp_path):
|
|
outside = tmp_path.parent / "outside.txt"
|
|
|
|
with pytest.raises(ValueError) as exc_info:
|
|
validate_file_path(str(outside), str(tmp_path))
|
|
|
|
message = str(exc_info.value)
|
|
assert "outside.txt" in message
|
|
assert str(tmp_path) not in message
|
|
assert str(tmp_path.parent) not in message
|
|
|
|
|
|
class TestFormatPathForDisplay:
|
|
"""Tests for user-visible path labels."""
|
|
|
|
def test_returns_relative_path_inside_base(self, tmp_path):
|
|
nested_file = tmp_path / "nested" / "file.txt"
|
|
nested_file.parent.mkdir()
|
|
nested_file.touch()
|
|
|
|
result = format_path_for_display(str(nested_file), str(tmp_path))
|
|
|
|
assert result == os.path.join("nested", "file.txt")
|
|
|
|
def test_redacts_absolute_prefix_outside_base(self, tmp_path):
|
|
outside_file = tmp_path.parent / "outside.txt"
|
|
|
|
result = format_path_for_display(str(outside_file), str(tmp_path))
|
|
|
|
assert result == "outside.txt"
|
|
|
|
|
|
class TestValidateDirectoryPath:
|
|
"""Tests for validate_directory_path."""
|
|
|
|
def test_valid_directory(self, tmp_path):
|
|
(tmp_path / "subdir").mkdir()
|
|
result = validate_directory_path("subdir", str(tmp_path))
|
|
assert result == str(tmp_path / "subdir")
|
|
|
|
def test_rejects_file_as_directory(self, tmp_path):
|
|
(tmp_path / "file.txt").touch()
|
|
with pytest.raises(ValueError, match="not a directory"):
|
|
validate_directory_path("file.txt", str(tmp_path))
|
|
|
|
def test_rejects_traversal(self, tmp_path):
|
|
with pytest.raises(ValueError, match="outside the allowed directory"):
|
|
validate_directory_path("../../", str(tmp_path))
|
|
|
|
|
|
class TestValidateUrl:
|
|
"""Tests for validate_url."""
|
|
|
|
def test_valid_https_url(self):
|
|
"""Normal HTTPS URL should pass."""
|
|
result = validate_url("https://example.com/data.json")
|
|
assert result == "https://example.com/data.json"
|
|
|
|
def test_valid_http_url(self):
|
|
"""Normal HTTP URL should pass."""
|
|
result = validate_url("http://example.com/api")
|
|
assert result == "http://example.com/api"
|
|
|
|
def test_blocks_file_scheme(self):
|
|
"""file:// URLs must be blocked."""
|
|
with pytest.raises(ValueError, match="file:// URLs are not allowed"):
|
|
validate_url("file:///etc/passwd")
|
|
|
|
def test_blocks_file_scheme_with_host(self):
|
|
with pytest.raises(ValueError, match="file:// URLs are not allowed"):
|
|
validate_url("file://localhost/etc/shadow")
|
|
|
|
def test_blocks_localhost(self):
|
|
"""localhost must be blocked (resolves to 127.0.0.1)."""
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://localhost/admin")
|
|
|
|
def test_blocks_127_0_0_1(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://127.0.0.1/admin")
|
|
|
|
def test_blocks_cloud_metadata(self):
|
|
"""AWS/GCP/Azure metadata endpoint must be blocked."""
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://169.254.169.254/latest/meta-data/")
|
|
|
|
def test_blocks_private_10_range(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://10.0.0.1/internal")
|
|
|
|
def test_blocks_private_172_range(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://172.16.0.1/internal")
|
|
|
|
def test_blocks_private_192_range(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://192.168.1.1/router")
|
|
|
|
def test_blocks_zero_address(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://0.0.0.0/")
|
|
|
|
def test_blocks_ipv6_localhost(self):
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
validate_url("http://[::1]/admin")
|
|
|
|
def test_blocks_ftp_scheme(self):
|
|
with pytest.raises(ValueError, match="not allowed"):
|
|
validate_url("ftp://example.com/file")
|
|
|
|
def test_blocks_empty_hostname(self):
|
|
with pytest.raises(ValueError, match="no hostname"):
|
|
validate_url("http:///path")
|
|
|
|
def test_blocks_unresolvable_host(self):
|
|
with pytest.raises(ValueError, match="Could not resolve"):
|
|
validate_url("http://this-host-definitely-does-not-exist-abc123.com/")
|
|
|
|
def test_escape_hatch(self, monkeypatch):
|
|
"""CREWAI_TOOLS_ALLOW_UNSAFE_PATHS=true bypasses URL validation."""
|
|
monkeypatch.setenv("CREWAI_TOOLS_ALLOW_UNSAFE_PATHS", "true")
|
|
# file:// would normally be blocked
|
|
result = validate_url("file:///etc/passwd")
|
|
assert result == "file:///etc/passwd"
|
|
|
|
|
|
class TestFormatSandboxError:
|
|
def test_replaces_bypass_advice_with_remedy(self, tmp_path):
|
|
with pytest.raises(ValueError) as exc:
|
|
validate_file_path(str(tmp_path.parent / "outside.txt"), str(tmp_path))
|
|
|
|
message = format_sandbox_error(exc.value, "Pass base_dir to widen it.")
|
|
|
|
assert "outside the allowed directory" in message
|
|
assert "Pass base_dir to widen it." in message
|
|
assert "CREWAI_TOOLS_ALLOW_UNSAFE_PATHS" not in message
|
|
|
|
def test_leaves_unrelated_errors_intact(self):
|
|
message = format_sandbox_error(ValueError("something else"), "Do this.")
|
|
|
|
assert message == "something else Do this."
|