fix(skills): bound streamed ClawHub ZIP downloads (#57571)

ClawHub ZIP downloads buffered the entire response before applying member
limits. Stream the archive into a 25 MiB bounded buffer and enforce actual
received bytes even when Content-Length is absent or incorrect.

Use the existing SSRF-safe client with bounded redirects and recheck URL and
website policy at every hop. Close responses before retry delays, clamp
Retry-After, and stop after the third rate-limited response without attempting
ZIP extraction. Preserve member path validation and raw-file fallback.

Related #29450
Co-authored-by: sprmn <oncuevtv@gmail.com>
Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>

tests/tools/test_clawhub_zip_stream.py

@@ -0,0 +1,110 @@
+"""Bound the archive before extraction, without buffering response.content."""
+
+from contextlib import contextmanager
+import io
+import zipfile
+
+import httpx
+import pytest
+
+from tools import skills_hub_clawhub as clawhub
+
+
+def _archive():
+    data = io.BytesIO()
+    with zipfile.ZipFile(data, "w") as archive:
+        archive.writestr("SKILL.md", "# A normal skill")
+    return data.getvalue()
+
+
+def _mock_download(monkeypatch, data, headers):
+    responses = []
+
+    @contextmanager
+    def stream(*args, **kwargs):
+        response = httpx.Response(200, headers=headers, stream=httpx.ByteStream(data))
+        responses.append(response)
+        try:
+            yield response
+        finally:
+            response.close()
+
+    monkeypatch.setattr(clawhub, "_guarded_http_stream", stream, raising=False)
+    monkeypatch.setattr(clawhub.httpx, "get", lambda *a, **k: httpx.Response(200, headers=headers, content=data))
+    return responses
+
+
+@pytest.mark.parametrize("declared", [None, "1", "invalid", "-1"])
+def test_actual_stream_size_enforces_cap_despite_header(monkeypatch, declared):
+    data = _archive()
+    _mock_download(monkeypatch, data, {} if declared is None else {"content-length": declared})
+    monkeypatch.setattr(clawhub.ClawHubSource, "ZIP_DOWNLOAD_MAX_BYTES", len(data) - 1, raising=False)
+    assert clawhub.ClawHubSource()._download_zip("example", "1") == {}
+
+
+def test_exact_limit_stream_extracts_without_content_access_and_closes(monkeypatch):
+    data = _archive()
+    responses = _mock_download(monkeypatch, data, {})
+    monkeypatch.setattr(clawhub.ClawHubSource, "ZIP_DOWNLOAD_MAX_BYTES", len(data), raising=False)
+    assert clawhub.ClawHubSource()._download_zip("example", "1") == {"SKILL.md": "# A normal skill"}
+    assert len(responses) == 1 and responses[0].is_closed
+
+
+def test_declared_oversize_does_not_read_body(monkeypatch):
+    responses = _mock_download(monkeypatch, b"", {"content-length": "101"})
+    monkeypatch.setattr(clawhub.ClawHubSource, "ZIP_DOWNLOAD_MAX_BYTES", 100)
+    monkeypatch.setattr(httpx.Response, "iter_bytes", lambda *a, **k: pytest.fail("read oversized body"))
+    assert clawhub.ClawHubSource()._download_zip("example", "1") == {}
+    assert responses[0].is_closed
+
+
+def test_rate_limit_exhaustion_closes_responses_and_sleeps_only_between_attempts(monkeypatch):
+    responses = []
+    delays = []
+
+    @contextmanager
+    def stream(*args, **kwargs):
+        response = httpx.Response(429, headers={"retry-after": ["-1", "1000", "invalid"][len(responses)]})
+        responses.append(response)
+        try:
+            yield response
+        finally:
+            response.close()
+
+    monkeypatch.setattr(clawhub, "_guarded_http_stream", stream)
+    monkeypatch.setattr(clawhub.time, "sleep", delays.append)
+    assert clawhub.ClawHubSource()._download_zip("example", "1") == {}
+    assert delays == [0, 15]
+    assert len(responses) == 3 and all(response.is_closed for response in responses)
+
+
+@pytest.mark.parametrize("destination", ["https://download.example/bundle", "http://127.0.0.1/private", "https://blocked.example/bundle"])
+def test_stream_rechecks_redirect_policy_and_closes_clients(monkeypatch, destination):
+    from tools import skills_hub, url_safety
+
+    requests = []
+    clients = []
+
+    def respond(request):
+        requests.append(request)
+        if len(requests) == 1:
+            return httpx.Response(302, headers={"location": destination})
+        return httpx.Response(200, content=b"bundle")
+
+    def client(**kwargs):
+        result = httpx.Client(transport=httpx.MockTransport(respond), **kwargs)
+        clients.append(result)
+        return result
+
+    monkeypatch.setattr(url_safety, "create_ssrf_safe_client", client)
+    monkeypatch.setattr(skills_hub, "is_safe_url", lambda url: "127.0.0.1" not in url)
+    monkeypatch.setattr(skills_hub, "check_website_access", lambda url: {"host": "blocked.example", "rule": "test"} if "blocked.example" in url else None)
+    with skills_hub._guarded_http_stream("https://api.example/download", params={"slug": "secret"}) as response:
+        if "download.example" in destination:
+            assert response.read() == b"bundle"
+            assert str(requests[1].url) == destination
+        else:
+            assert response is None
+            assert len(requests) == 1
+    assert requests[0].url.params["slug"] == "secret"
+    assert clients and all(client.is_closed for client in clients)

tests/tools/test_skills_hub_clawhub.py

@@ -158,9 +158,12 @@ class TestClawHubSource(unittest.TestCase):
         self.assertIsNotNone(meta)
         self.assertNotIn("owner", meta.extra or {})
 
+    @patch("tools.skills_hub_clawhub._guarded_http_stream")
     @patch("tools.skills_hub._ssrf_safe_http_get")
     @patch("tools.skills_hub.httpx.get")
-    def test_fetch_resolves_latest_version_and_downloads_raw_files(self, mock_get, mock_safe_get):
+    def test_fetch_resolves_latest_version_and_downloads_raw_files(
+        self, mock_get, mock_safe_get, mock_stream
+    ):
         def side_effect(url, *args, **kwargs):
             if url.endswith("/skills/caldav-calendar"):
                 return _MockResponse(
@@ -184,6 +187,7 @@ class TestClawHubSource(unittest.TestCase):
 
         mock_get.side_effect = side_effect
         mock_safe_get.return_value = _MockResponse(status_code=200, text="# Skill")
+        mock_stream.return_value.__enter__.return_value = _MockResponse(status_code=404)
 
         bundle = self.src.fetch("caldav-calendar")
 
@@ -211,11 +215,14 @@ class TestClawHubSource(unittest.TestCase):
         self.assertIsNotNone(bundle)
         self.assertEqual(bundle.files["SKILL.md"], "# Skill")
 
+    @patch("tools.skills_hub_clawhub._guarded_http_stream")
     @patch("tools.skills_hub.check_website_access", return_value=None)
     @patch("tools.skills_hub.is_safe_url")
     @patch("tools.skills_hub.httpx.get")
     @patch("tools.skills_hub._ssrf_safe_http_get")
-    def test_fetch_blocks_private_raw_url(self, mock_safe_get, mock_get, mock_safe, _mock_policy):
+    def test_fetch_blocks_private_raw_url(
+        self, mock_safe_get, mock_get, mock_safe, _mock_policy, mock_stream
+    ):
         def side_effect(url, *args, **kwargs):
             if url.endswith("/skills/caldav-calendar"):
                 return _MockResponse(
@@ -240,11 +247,12 @@ class TestClawHubSource(unittest.TestCase):
 
         mock_get.side_effect = side_effect
         mock_safe.side_effect = lambda url: not url.startswith("http://127.0.0.1/")
+        mock_stream.return_value.__enter__.return_value = _MockResponse(status_code=404)
 
         bundle = self.src.fetch("caldav-calendar")
 
         self.assertIsNone(bundle)
-        self.assertEqual(mock_get.call_count, 3)
+        self.assertEqual(mock_get.call_count, 2)
         mock_safe_get.assert_not_called()
 
     @patch("tools.skills_hub._write_index_cache")

tools/skills_hub.py

@@ -14,9 +14,10 @@ Used by hermes_cli/skills_hub.py for CLI commands and the /skills slash command.
 import json
 import logging
 import time
+from contextlib import ExitStack, contextmanager
 from datetime import datetime, timezone
 from pathlib import Path
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, Iterator, List, Optional
 from urllib.parse import urljoin
 
 import httpx
@@ -130,6 +131,70 @@ def _guarded_http_get(url: str, *, timeout: int = 20) -> Optional[httpx.Response
     return None
 
 
+@contextmanager
+def _guarded_http_stream(
+    url: str,
+    *,
+    params: Optional[Dict[str, str]] = None,
+    timeout: int = 20,
+) -> Iterator[Optional[httpx.Response]]:
+    """Stream one response with bounded, policy-checked redirects."""
+    from tools.url_safety import SSRFConnectionBlocked, create_ssrf_safe_client
+
+    current_url = url
+    current_params = params
+    response: Optional[httpx.Response] = None
+    stack = ExitStack()
+
+    try:
+        for _ in range(_MAX_SKILL_FETCH_REDIRECTS + 1):
+            if not is_safe_url(current_url):
+                logger.warning("Blocked unsafe Skills Hub URL: %s", current_url)
+                response = None
+                break
+
+            blocked = check_website_access(current_url)
+            if blocked:
+                logger.info(
+                    "Blocked Skills Hub fetch for %s by rule %s",
+                    blocked["host"],
+                    blocked["rule"],
+                )
+                response = None
+                break
+
+            stack.close()
+            stack = ExitStack()
+            try:
+                client = stack.enter_context(
+                    create_ssrf_safe_client(timeout=timeout, follow_redirects=False)
+                )
+                response = stack.enter_context(
+                    client.stream("GET", current_url, params=current_params)
+                )
+            except (SSRFConnectionBlocked, httpx.HTTPError) as exc:
+                logger.debug("Skills Hub stream failed for %s: %s", current_url, exc)
+                response = None
+                break
+
+            if response.status_code not in _REDIRECT_STATUS_CODES:
+                break
+
+            location = response.headers.get("location")
+            if not location:
+                response = None
+                break
+            current_url = urljoin(current_url, location)
+            current_params = None
+        else:
+            logger.warning("Skills Hub fetch exceeded redirect limit for %s", url)
+            response = None
+
+        yield response
+    finally:
+        stack.close()
+
+
 # ---------------------------------------------------------------------------
 # Shared index cache (used by every adapter)
 # ---------------------------------------------------------------------------

tools/skills_hub_clawhub.py

@@ -9,6 +9,7 @@ from typing import Any, Dict, List, Optional, Tuple
 
 import httpx
 
+from tools.skills_hub import _guarded_http_stream
 from tools.skills_hub_models import (
     GuardedFetchMixin, SkillBundle, SkillMeta, SkillSource, _cache_metas, _cached_metas, _get_json,
     _validate_bundle_rel_path,
@@ -65,6 +66,8 @@ class ClawHubSource(GuardedFetchMixin, SkillSource):
     # Wall-clock budget for a full catalog walk: 50k+ skills, sequential
     # (~250 requests each under timeout=30), so unbounded it blocks for minutes.
     CATALOG_WALK_BUDGET_SECONDS = 12
+    ZIP_DOWNLOAD_MAX_BYTES = 25 * 1024 * 1024
+    ZIP_DOWNLOAD_CHUNK_BYTES = 64 * 1024
     _SLUG_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._-]*$")
 
     _query_terms = staticmethod(_query_terms)
@@ -468,7 +471,7 @@ class ClawHubSource(GuardedFetchMixin, SkillSource):
         return files
 
     def _download_zip(self, slug: str, version: str, owner: Optional[str] = None) -> Dict[str, str]:
-        """Download the skill ZIP from /download and extract its text files."""
+        """Download the skill ZIP from /download (bounded, streamed) and extract its text files."""
         import io
         import zipfile
 
@@ -478,22 +481,65 @@ class ClawHubSource(GuardedFetchMixin, SkillSource):
             params["owner"] = owner
         max_retries = 3
         for attempt in range(max_retries):
+            retry_after_delay: Optional[int] = None
             try:
-                resp = httpx.get(f"{self.BASE_URL}/download", params=params,
-                                 timeout=30, follow_redirects=True)
-                if resp.status_code == 429:
-                    try:
-                        retry_after = min(int(resp.headers.get("retry-after", "5")), 15)  # Cap wait time
-                    except (ValueError, TypeError):
-                        retry_after = 5
-                    logger.debug("ClawHub download rate-limited for %s, retrying in %ds (attempt %d/%d)",
-                                 slug, retry_after, attempt + 1, max_retries)
-                    time.sleep(retry_after)
+                with _guarded_http_stream(
+                    f"{self.BASE_URL}/download",
+                    params=params,
+                    timeout=30,
+                ) as resp:
+                    if resp is None:
+                        return files
+                    if resp.status_code == 429:
+                        try:
+                            retry_after = int(resp.headers.get("retry-after", "5"))
+                        except (ValueError, TypeError):
+                            retry_after = 5
+                        retry_after = max(0, min(retry_after, 15))  # Cap wait time
+                        logger.debug(
+                            "ClawHub download rate-limited for %s, retrying in %ds (attempt %d/%d)",
+                            slug, retry_after, attempt + 1, max_retries,
+                        )
+                        retry_after_delay = retry_after
+                    else:
+                        if resp.status_code != 200:
+                            logger.debug("ClawHub ZIP download for %s v%s returned %s", slug, version, resp.status_code)
+                            return files
+
+                        content_length = resp.headers.get("content-length")
+                        if content_length:
+                            try:
+                                declared_size = int(content_length)
+                            except (ValueError, TypeError):
+                                declared_size = 0
+                            if declared_size > self.ZIP_DOWNLOAD_MAX_BYTES:
+                                logger.debug(
+                                    "Skipping oversized ClawHub ZIP for %s v%s: %d bytes",
+                                    slug, version, declared_size,
+                                )
+                                return files
+
+                        archive = io.BytesIO()
+                        total = 0
+                        for chunk in resp.iter_bytes(chunk_size=self.ZIP_DOWNLOAD_CHUNK_BYTES):
+                            if not chunk:
+                                continue
+                            total += len(chunk)
+                            if total > self.ZIP_DOWNLOAD_MAX_BYTES:
+                                logger.debug(
+                                    "Skipping oversized ClawHub ZIP for %s v%s: exceeded %d bytes",
+                                    slug, version, self.ZIP_DOWNLOAD_MAX_BYTES,
+                                )
+                                return files
+                            archive.write(chunk)
+                        archive.seek(0)
+
+                if retry_after_delay is not None:
+                    if attempt < max_retries - 1:
+                        time.sleep(retry_after_delay)
                     continue
-                if resp.status_code != 200:
-                    logger.debug("ClawHub ZIP download for %s v%s returned %s", slug, version, resp.status_code)
-                    return files
-                with zipfile.ZipFile(io.BytesIO(resp.content)) as zf:
+
+                with zipfile.ZipFile(archive) as zf:
                     for info in zf.infolist():
                         if info.is_dir():
                             continue