mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-08-11 08:51:43 +00:00
* feat(tools): add URLReadTool for reading arbitrary URLs FileReadTool is confined to the local filesystem, so there was no way for an agent to read a document that lives behind an http(s) URL. Rather than adding a flag to FileReadTool, this adds a separate tool: granting it grants network egress to addresses an LLM picks at runtime, and that should be a deliberate choice rather than a toggle on a filesystem tool. URLReadTool fetches a URL and returns its content as text. PDF and DOCX bodies have their text extracted, HTML is stripped to visible text, and text-shaped types (plain text, Markdown, JSON, XML, YAML, CSV) are decoded using the charset the server declares. Any other content type is refused rather than returned as base64, keeping the output text-only. Requests reuse the existing SSRF protections in security/safe_requests: validate_url resolves every hostname and rejects private, loopback, link-local and reserved addresses (covering cloud metadata endpoints), and safe_get never auto-follows redirects, revalidating each hop and dropping credentials on cross-origin ones. Resolving before validating also normalizes encoded forms, so http://2130706433/ is rejected as 127.0.0.1 without needing a string blocklist. Adds safe_get_bounded on top of that, which streams the body and abandons it once it crosses max_bytes. The cap counts decoded bytes, which is what a compressed response expands into -- Content-Length describes the wire size and cannot bound that. It also closes the redirect hops, which stream=True would otherwise leave holding their connections. Two risks are documented rather than closed. Validation resolves the hostname and requests resolves it again to connect, so DNS rebinding remains possible; closing it needs the connection pinned to the validated address, which would change behavior for all existing safe_get callers. And the returned text is untrusted remote content entering an agent's context, which input validation cannot address. Also fixes a temp file leak in PDFLoader, which reached the same pymupdf-from-URL path. It wrote downloads to NamedTemporaryFile with delete=False and never unlinked them, so every PDF ingested from a URL left a file behind. It now opens from memory, the way URLReadTool does, which removes the leak by construction instead of relying on cleanup on each error path; its doc.close() also moves into a finally so a failure mid-extraction still releases the handle. PDFLoader had no test file, so this adds one covering both paths plus a regression test asserting no temp file is created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tools): address review feedback on URLReadTool Bound start_line and line_count with ge=1 in the args schema. Both were unbounded, and _window computes stop as start + line_count, so a negative line_count reached islice, which rejects a negative stop. The windowing runs outside the tool's error handling, so that escaped _run as a raw ValueError instead of an error string. BaseTool.run validates kwargs against args_schema, so the constraint refuses the value before any request is made. _window also clamps its own bounds now, so it cannot raise if called directly. Bound the PDFLoader download with safe_get_bounded. The body is held in memory for the whole extraction, so it needed a ceiling; it defaults to 50 MiB and takes a max_bytes kwarg to load() for callers ingesting larger documents. Patch the loader's own seam in its tests rather than requests.get. Both safe_get and safe_get_bounded resolve the hostname before requesting, so the previous mocks made the tests depend on DNS for example.com and fail in a network-isolated runner for reasons unrelated to the loader. Exercise the content-type fallback through run() rather than asserting on _resolve_kind, so the tests survive a refactor of the classification internals, and cover the octet-stream PDF, missing-type, query-string and unknown-extension cases as observable behavior. Add docstrings to the new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tools): close streamed redirect hops and widen type fallback Four findings from the Copilot and Cursor reviews. safe_get leaked its accumulated hops on every failure path. It closed the response it was about to abandon but not the ones already in history, and a caller handed an exception has no handle on them -- under stream=True each holds its connection until its body is read or closed. The loop now closes history before re-raising. Hops are still the caller's on success, where they arrive via response.history. safe_get_bounded rejected a non-positive max_bytes only after issuing the request, and then reported it as an oversized body. It now fails before the request. Its oversized-body error also named the requested URL rather than the one that served the body, which differ after a redirect. The content-type fallback consulted only the final URL for an extension, so a .pdf link redirecting to an extensionless CDN or presigned path was refused even though the requested URL identified the type. It now checks the final URL first, then the requested one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
252 lines
8.1 KiB
Python
252 lines
8.1 KiB
Python
"""Tests for redirect-aware safe HTTP helpers."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import socket
|
|
from io import BytesIO
|
|
from typing import Any
|
|
|
|
import pytest
|
|
import requests
|
|
|
|
from crewai_tools.security.safe_requests import safe_get
|
|
|
|
|
|
def _response(url: str, status_code: int, *, location: str | None = None) -> requests.Response:
|
|
response = requests.Response()
|
|
response.status_code = status_code
|
|
response.url = url
|
|
response._content = b"ok"
|
|
response.raw = BytesIO()
|
|
if location is not None:
|
|
response.headers["Location"] = location
|
|
return response
|
|
|
|
|
|
@pytest.fixture
|
|
def public_dns(monkeypatch: pytest.MonkeyPatch) -> None:
|
|
original_getaddrinfo = socket.getaddrinfo
|
|
|
|
def fake_getaddrinfo(
|
|
host: str, port: int, *args: Any, **kwargs: Any
|
|
) -> list[tuple[Any, ...]]:
|
|
if host in {"public.example", "safe.example"}:
|
|
return [
|
|
(
|
|
socket.AF_INET,
|
|
socket.SOCK_STREAM,
|
|
6,
|
|
"",
|
|
("93.184.216.34", port),
|
|
)
|
|
]
|
|
return original_getaddrinfo(host, port, *args, **kwargs)
|
|
|
|
monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo)
|
|
|
|
|
|
def test_safe_get_blocks_direct_internal_url() -> None:
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
safe_get("http://127.0.0.1/admin", timeout=15)
|
|
|
|
|
|
def _mock_get(monkeypatch: pytest.MonkeyPatch, get_response: Any) -> None:
|
|
monkeypatch.setattr(
|
|
"crewai_tools.security.safe_requests.requests.get",
|
|
get_response,
|
|
)
|
|
|
|
|
|
def test_safe_get_blocks_redirect_to_internal_url(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
requested_urls: list[str] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
requested_urls.append(url)
|
|
assert kwargs["allow_redirects"] is False
|
|
return _response(url, 302, location="http://127.0.0.1/admin")
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
safe_get("http://public.example/start", timeout=15)
|
|
|
|
assert requested_urls == ["http://public.example/start"]
|
|
|
|
|
|
def test_safe_get_follows_safe_relative_redirect(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
requested_urls: list[str] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
requested_urls.append(url)
|
|
assert kwargs["allow_redirects"] is False
|
|
if url == "http://public.example/start":
|
|
return _response(url, 302, location="/final")
|
|
return _response(url, 200)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
response = safe_get("http://public.example/start", timeout=15)
|
|
|
|
assert response.status_code == 200
|
|
assert response.url == "http://public.example/final"
|
|
assert requested_urls == [
|
|
"http://public.example/start",
|
|
"http://public.example/final",
|
|
]
|
|
assert len(response.history) == 1
|
|
|
|
|
|
def test_safe_get_fails_closed_after_too_many_redirects(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
return _response(url, 302, location="http://safe.example/again")
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
with pytest.raises(ValueError, match="Too many redirects"):
|
|
safe_get("http://public.example/start", max_redirects=1, timeout=15)
|
|
|
|
|
|
def _closable_response(
|
|
url: str, status_code: int, *, location: str | None = None, closed: list[str]
|
|
) -> requests.Response:
|
|
"""Build a response that records its own URL when closed."""
|
|
response = _response(url, status_code, location=location)
|
|
response.close = lambda: closed.append(url) # type: ignore[method-assign]
|
|
return response
|
|
|
|
|
|
def test_safe_get_closes_earlier_hops_after_too_many_redirects(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
"""Hops accumulated before the failure must not be left open.
|
|
|
|
Under stream=True each hop holds its connection until its body is read or
|
|
closed, and a caller handed an exception has no handle on them.
|
|
"""
|
|
closed: list[str] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
return _closable_response(
|
|
url, 302, location="http://safe.example/again", closed=closed
|
|
)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
with pytest.raises(ValueError, match="Too many redirects"):
|
|
safe_get("http://public.example/start", max_redirects=2, timeout=15, stream=True)
|
|
|
|
assert len(closed) == 3
|
|
|
|
|
|
def test_safe_get_closes_earlier_hops_when_a_redirect_is_rejected(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
"""A hop rejected mid-chain still releases the connections already open."""
|
|
closed: list[str] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
if url == "http://public.example/start":
|
|
return _closable_response(
|
|
url, 302, location="http://safe.example/next", closed=closed
|
|
)
|
|
return _closable_response(
|
|
url, 302, location="http://169.254.169.254/latest", closed=closed
|
|
)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
with pytest.raises(ValueError, match="private/reserved IP"):
|
|
safe_get("http://public.example/start", timeout=15, stream=True)
|
|
|
|
assert closed == ["http://safe.example/next", "http://public.example/start"]
|
|
|
|
|
|
def test_safe_get_leaves_hops_open_on_success(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
"""On success the hops belong to the caller, via response.history."""
|
|
closed: list[str] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
if url == "http://public.example/start":
|
|
return _closable_response(url, 302, location="/final", closed=closed)
|
|
return _closable_response(url, 200, closed=closed)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
response = safe_get("http://public.example/start", timeout=15, stream=True)
|
|
|
|
assert closed == []
|
|
assert len(response.history) == 1
|
|
|
|
|
|
def test_safe_get_strips_credentials_on_cross_origin_redirect(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
requests_made: list[tuple[str, dict[str, Any]]] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
requests_made.append((url, kwargs))
|
|
if url == "http://public.example/start":
|
|
return _response(url, 302, location="http://safe.example/final")
|
|
return _response(url, 200)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
response = safe_get(
|
|
"http://public.example/start",
|
|
timeout=15,
|
|
headers={
|
|
"Authorization": "Bearer token",
|
|
"Authorization-Custom": "secret token",
|
|
"Cookie": "session=abc",
|
|
"X-API-Key": "api key",
|
|
"X-CrewAI-Token": "crewai token",
|
|
"User-Agent": "crewai-test",
|
|
},
|
|
cookies={"session": "abc"},
|
|
)
|
|
|
|
assert response.status_code == 200
|
|
assert requests_made[0][1]["headers"] == {
|
|
"Authorization": "Bearer token",
|
|
"Authorization-Custom": "secret token",
|
|
"Cookie": "session=abc",
|
|
"X-API-Key": "api key",
|
|
"X-CrewAI-Token": "crewai token",
|
|
"User-Agent": "crewai-test",
|
|
}
|
|
assert requests_made[0][1]["cookies"] == {"session": "abc"}
|
|
assert requests_made[1][1]["headers"] == {"User-Agent": "crewai-test"}
|
|
assert "cookies" not in requests_made[1][1]
|
|
|
|
|
|
def test_safe_get_preserves_credentials_on_same_origin_redirect(
|
|
monkeypatch: pytest.MonkeyPatch, public_dns: None
|
|
) -> None:
|
|
requests_made: list[tuple[str, dict[str, Any]]] = []
|
|
|
|
def fake_get(url: str, **kwargs: Any) -> requests.Response:
|
|
requests_made.append((url, kwargs))
|
|
if url == "http://public.example/start":
|
|
return _response(url, 302, location="/final")
|
|
return _response(url, 200)
|
|
|
|
_mock_get(monkeypatch, fake_get)
|
|
|
|
safe_get(
|
|
"http://public.example/start",
|
|
timeout=15,
|
|
headers={"Authorization": "Bearer token"},
|
|
cookies={"session": "abc"},
|
|
)
|
|
|
|
assert requests_made[1][1]["headers"] == {"Authorization": "Bearer token"}
|
|
assert requests_made[1][1]["cookies"] == {"session": "abc"}
|