diff --git a/tests/tools/test_clawhub_zip_stream.py b/tests/tools/test_clawhub_zip_stream.py new file mode 100644 index 0000000000..f66147237a --- /dev/null +++ b/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) diff --git a/tests/tools/test_skills_hub_clawhub.py b/tests/tools/test_skills_hub_clawhub.py index 9ef45dff8b..ab5de69cc6 100644 --- a/tests/tools/test_skills_hub_clawhub.py +++ b/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") diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 7de6260374..6fafdddabd 100644 --- a/tools/skills_hub.py +++ b/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) # --------------------------------------------------------------------------- diff --git a/tools/skills_hub_clawhub.py b/tools/skills_hub_clawhub.py index e7e710bd84..f56c83243d 100644 --- a/tools/skills_hub_clawhub.py +++ b/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