fix(tools): close streamed redirect hops and widen type fallback

joaomdmoura · claude · joaomdmoura · commit fa3889853244 · 2026-08-05T15:29:47.000-07:00
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) &lt;noreply@anthropic.com&gt;
diff --git a/lib/crewai-tools/src/crewai_tools/security/safe_requests.py b/lib/crewai-tools/src/crewai_tools/security/safe_requests.py
@@ -50,43 +50,54 @@ def _strip_cross_origin_credentials(request_kwargs: dict[str, Any]) -> dict[str,
 
 
 def safe_get(url: str, *, max_redirects: int = 10, **kwargs: Any) -> requests.Response:
-    """GET a URL while validating each redirect target before following it."""
+    """GET a URL while validating each redirect target before following it.
+
+    On success the hops are attached to the returned response's ``history`` and
+    are the caller's to close. On failure they are closed here: a caller given
+    an exception has no handle on them, and a streamed hop holds its connection
+    until its body is read or closed.
+    """
     current_url = validate_url(url)
     request_kwargs = {**kwargs, "allow_redirects": False}
     timeout = request_kwargs.pop("timeout", 30)
     history: list[requests.Response] = []
     redirects_followed = 0
 
-    while True:
-        response = requests.get(current_url, timeout=timeout, **request_kwargs)
-        if (
-            response.status_code not in _REDIRECT_STATUS_CODES
-            or "Location" not in response.headers
-        ):
-            response.history = history
-            return response
-
-        if redirects_followed >= max_redirects:
-            response.close()
-            raise ValueError(f"Too many redirects while fetching URL: {url}")
-
-        location = response.headers.get("Location")
-        if not location:
-            response.history = history
-            return response
-
-        try:
-            redirect_url = validate_url(urljoin(response.url, location))
-        except ValueError:
-            response.close()
-            raise
-
-        if not _same_origin(current_url, redirect_url):
-            request_kwargs = _strip_cross_origin_credentials(request_kwargs)
-
-        history.append(response)
-        current_url = redirect_url
-        redirects_followed += 1
+    try:
+        while True:
+            response = requests.get(current_url, timeout=timeout, **request_kwargs)
+            if (
+                response.status_code not in _REDIRECT_STATUS_CODES
+                or "Location" not in response.headers
+            ):
+                response.history = history
+                return response
+
+            if redirects_followed >= max_redirects:
+                response.close()
+                raise ValueError(f"Too many redirects while fetching URL: {url}")
+
+            location = response.headers.get("Location")
+            if not location:
+                response.history = history
+                return response
+
+            try:
+                redirect_url = validate_url(urljoin(response.url, location))
+            except ValueError:
+                response.close()
+                raise
+
+            if not _same_origin(current_url, redirect_url):
+                request_kwargs = _strip_cross_origin_credentials(request_kwargs)
+
+            history.append(response)
+            current_url = redirect_url
+            redirects_followed += 1
+    except BaseException:
+        for hop in history:
+            hop.close()
+        raise
 
 
 def safe_get_bounded(
@@ -116,11 +127,14 @@ def safe_get_bounded(
         last validated URL in the redirect chain.
 
     Raises:
-        ValueError: If URL validation fails, the redirect chain is too long, or
-            the body exceeds *max_bytes*.
+        ValueError: If *max_bytes* is not positive, URL validation fails, the
+            redirect chain is too long, or the body exceeds *max_bytes*.
         requests.RequestException: If the request fails or returns an error
             status.
     """
+    if max_bytes <= 0:
+        raise ValueError(f"max_bytes must be positive, got {max_bytes}.")
+
     response = safe_get(
         url,
         max_redirects=max_redirects,
@@ -138,8 +152,11 @@ def safe_get_bounded(
                 continue
             total += len(chunk)
             if total > max_bytes:
+                # Names the URL that served the body, which after a redirect is
+                # not the one that was requested.
                 raise ValueError(
-                    f"Response body from '{url}' exceeds the {max_bytes} byte limit."
+                    f"Response body from '{response.url}' exceeds the "
+                    f"{max_bytes} byte limit."
                 )
             chunks.append(chunk)
 
diff --git a/lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py b/lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py
@@ -203,12 +203,15 @@ def _classify(media_type: str) -> str | None:
             return "text"
         return None
 
-    def _resolve_kind(self, content_type: str, url: str) -> str | None:
+    def _resolve_kind(self, content_type: str, *urls: str) -> str | None:
         """Decide how to extract text, by content type then by URL extension.
 
         Args:
             content_type: The raw Content-Type header value.
-            url: The final URL of the response.
+            *urls: URLs to consult for an extension, most authoritative first.
+                A ``.pdf`` link that redirects to an extensionless CDN or
+                presigned path only carries its type on the requested URL, so
+                both ends of the chain are worth checking.
 
         Returns:
             The extractor name, or None when the content type is unsupported.
@@ -217,10 +220,11 @@ def _resolve_kind(self, content_type: str, url: str) -> str | None:
         if declared not in _UNINFORMATIVE_TYPES:
             return self._classify(declared)
 
-        path = urlparse(url).path.lower()
-        for extension, media_type in _EXTENSION_TYPES.items():
-            if path.endswith(extension):
-                return self._classify(media_type)
+        for url in urls:
+            path = urlparse(url).path.lower()
+            for extension, media_type in _EXTENSION_TYPES.items():
+                if path.endswith(extension):
+                    return self._classify(media_type)
         return None
 
     def _decode(self, body: bytes, content_type: str) -> str:
@@ -353,7 +357,7 @@ def _run(
         except requests.RequestException as e:
             return f"Error: Failed to fetch '{url}'. {format_error_for_display(e)}"
 
-        kind = self._resolve_kind(content_type, final_url)
+        kind = self._resolve_kind(content_type, final_url, url)
         if kind is None:
             return (
                 f"Error: Unsupported content type "
diff --git a/lib/crewai-tools/tests/url_read_tool_test.py b/lib/crewai-tools/tests/url_read_tool_test.py
@@ -216,6 +216,24 @@ def test_query_string_does_not_break_extension_fallback():
         assert tool.run(url=url) == "a,b\n"
 
 
+def test_extension_from_requested_url_survives_a_redirect():
+    """A .pdf link that redirects to an extensionless path is still extracted.
+
+    Presigned CDN targets routinely drop the extension and serve octet-stream,
+    so the requested URL is the only place the type survives.
+    """
+    tool = URLReadTool()
+    with patch(f"{TOOL_MODULE}.safe_get_bounded") as fetch:
+        fetch.return_value = fetch_result(
+            build_pdf("Survived the redirect"),
+            "application/octet-stream",
+            "https://cdn.example.com/objects/9f8a7b6c5d",
+        )
+        result = tool.run(url="https://example.com/report.pdf")
+
+    assert "Survived the redirect" in result
+
+
 def test_octet_stream_with_unknown_extension_is_rejected():
     """With neither a usable type nor a known extension, the read is refused."""
     tool = URLReadTool()
@@ -322,6 +340,26 @@ def test_rejects_body_over_the_limit(self):
 
         assert response.closed
 
+    def test_oversized_error_names_the_url_that_served_the_body(self):
+        """After a redirect the requested URL is not the one that sent it."""
+        response = FakeResponse(
+            b"x" * 100, url="https://cdn.example.com/final", chunk_size=10
+        )
+        with patch(
+            "crewai_tools.security.safe_requests.safe_get", return_value=response
+        ):
+            with pytest.raises(ValueError, match="https://cdn.example.com/final"):
+                safe_get_bounded("https://example.com/start", max_bytes=25)
+
+    @pytest.mark.parametrize("max_bytes", [0, -1])
+    def test_non_positive_max_bytes_fails_before_requesting(self, max_bytes):
+        """A misconfigured cap is caught without issuing a request."""
+        with patch("crewai_tools.security.safe_requests.safe_get") as safe_get:
+            with pytest.raises(ValueError, match="max_bytes must be positive"):
+                safe_get_bounded("https://example.com/f", max_bytes=max_bytes)
+
+        safe_get.assert_not_called()
+
     def test_stops_reading_once_the_limit_is_crossed(self):
         """The cap must abandon the stream, not buffer the whole body first."""
         chunks_yielded = 0
diff --git a/lib/crewai-tools/tests/utilities/test_safe_requests.py b/lib/crewai-tools/tests/utilities/test_safe_requests.py
@@ -112,6 +112,80 @@ def fake_get(url: str, **kwargs: Any) -> requests.Response:
         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:
