diff --git a/tests/tools/test_skills_hub_clawhub.py b/tests/tools/test_skills_hub_clawhub.py index 42d2edbbcbc1..d6d85ef98695 100644 --- a/tests/tools/test_skills_hub_clawhub.py +++ b/tests/tools/test_skills_hub_clawhub.py @@ -32,11 +32,12 @@ def tearDown(self): @patch("tools.skills_hub._write_index_cache") @patch("tools.skills_hub._read_index_cache", return_value=None) @patch.object(ClawHubSource, "_load_catalog_index", return_value=[]) + @patch("tools.skills_hub._ssrf_safe_http_get") @patch("tools.skills_hub.httpx.get") def test_search_uses_listing_endpoint_as_fallback( - self, mock_get, _mock_load_catalog, _mock_read_cache, _mock_write_cache + self, mock_get, mock_safe_get, _mock_load_catalog, _mock_read_cache, _mock_write_cache ): - def side_effect(url, *args, **kwargs): + def listing_side_effect(url, *args, **kwargs): if url.endswith("/skills"): return _MockResponse( status_code=200, @@ -51,11 +52,11 @@ def side_effect(url, *args, **kwargs): ] }, ) - if url.endswith("/skills/caldav"): - return _MockResponse(status_code=404, json_data={}) return _MockResponse(status_code=404, json_data={}) - mock_get.side_effect = side_effect + # Exact-slug probes go through _guarded_http_get -> _ssrf_safe_http_get. + mock_safe_get.return_value = _MockResponse(status_code=404, json_data={}) + mock_get.side_effect = listing_side_effect results = self.src.search("caldav", limit=5) @@ -64,7 +65,7 @@ def side_effect(url, *args, **kwargs): self.assertEqual(results[0].name, "CalDAV Calendar") self.assertEqual(results[0].description, "Calendar integration") - self.assertGreaterEqual(mock_get.call_count, 2) + self.assertGreaterEqual(mock_get.call_count, 1) args, kwargs = mock_get.call_args_list[0] self.assertTrue(args[0].endswith("/skills")) self.assertEqual(kwargs["params"], {"search": "caldav", "limit": 5}) @@ -76,11 +77,12 @@ def side_effect(url, *args, **kwargs): "_load_catalog_index", return_value=[], ) + @patch("tools.skills_hub._ssrf_safe_http_get") @patch("tools.skills_hub.httpx.get") def test_search_falls_back_to_exact_slug_when_search_results_are_irrelevant( - self, mock_get, _mock_load_catalog, _mock_read_cache, _mock_write_cache + self, mock_get, mock_safe_get, _mock_load_catalog, _mock_read_cache, _mock_write_cache ): - def side_effect(url, *args, **kwargs): + def listing_side_effect(url, *args, **kwargs): if url.endswith("/skills"): return _MockResponse( status_code=200, @@ -94,6 +96,9 @@ def side_effect(url, *args, **kwargs): ] }, ) + return _MockResponse(status_code=404, json_data={}) + + def safe_side_effect(url, *args, **kwargs): if url.endswith("/skills/self-improving-agent"): return _MockResponse( status_code=200, @@ -109,7 +114,8 @@ def side_effect(url, *args, **kwargs): ) return _MockResponse(status_code=404, json_data={}) - mock_get.side_effect = side_effect + mock_get.side_effect = listing_side_effect + mock_safe_get.side_effect = safe_side_effect results = self.src.search("self-improving-agent", limit=5) @@ -118,7 +124,7 @@ def side_effect(url, *args, **kwargs): self.assertEqual(results[0].name, "self-improving-agent") self.assertIn("continuous improvement", results[0].description) - @patch("tools.skills_hub.httpx.get") + @patch("tools.skills_hub._ssrf_safe_http_get") def test_search_repairs_poisoned_cache_with_exact_slug_lookup(self, mock_get): mock_get.return_value = _MockResponse( status_code=200, @@ -170,7 +176,7 @@ def test_search_matches_space_separated_query_to_hyphenated_slug( self.assertEqual(len(results), 1) self.assertEqual(results[0].identifier, "self-improving-agent") - @patch("tools.skills_hub.httpx.get") + @patch("tools.skills_hub._ssrf_safe_http_get") def test_inspect_maps_display_name_and_summary(self, mock_get): mock_get.return_value = _MockResponse( status_code=200, @@ -189,7 +195,7 @@ def test_inspect_maps_display_name_and_summary(self, mock_get): self.assertEqual(meta.description, "Calendar integration") self.assertEqual(meta.identifier, "caldav-calendar") - @patch("tools.skills_hub.httpx.get") + @patch("tools.skills_hub._ssrf_safe_http_get") def test_inspect_handles_nested_skill_payload(self, mock_get): mock_get.return_value = _MockResponse( status_code=200, @@ -213,9 +219,10 @@ def test_inspect_handles_nested_skill_payload(self, mock_get): self.assertEqual(meta.tags, ["automation"]) @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_safe_get): def side_effect(url, *args, **kwargs): + if "/download" in url: + return _MockResponse(status_code=404, json_data={}) if url.endswith("/skills/caldav-calendar"): return _MockResponse( status_code=200, @@ -234,10 +241,11 @@ def side_effect(url, *args, **kwargs): ] }, ) + if url == "https://files.example/skill-md": + return _MockResponse(status_code=200, text="# Skill") return _MockResponse(status_code=404, json_data={}) - mock_get.side_effect = side_effect - mock_safe_get.return_value = _MockResponse(status_code=200, text="# Skill") + mock_safe_get.side_effect = side_effect bundle = self.src.fetch("caldav-calendar") @@ -246,11 +254,13 @@ def side_effect(url, *args, **kwargs): self.assertIn("SKILL.md", bundle.files) self.assertEqual(bundle.files["SKILL.md"], "# Skill") self.assertEqual(bundle.files["README.md"], "hello") - mock_safe_get.assert_called_once_with("https://files.example/skill-md", timeout=20) + mock_safe_get.assert_any_call("https://files.example/skill-md", timeout=20) - @patch("tools.skills_hub.httpx.get") + @patch("tools.skills_hub._ssrf_safe_http_get") def test_fetch_falls_back_to_versions_list(self, mock_get): def side_effect(url, *args, **kwargs): + if "/download" in url: + return _MockResponse(status_code=404, json_data={}) if url.endswith("/skills/caldav-calendar"): return _MockResponse(status_code=200, json_data={"slug": "caldav-calendar"}) if url.endswith("/skills/caldav-calendar/versions"): @@ -267,9 +277,8 @@ def side_effect(url, *args, **kwargs): @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_safe, _mock_policy): def side_effect(url, *args, **kwargs): if url.endswith("/skills/caldav-calendar"): return _MockResponse( @@ -279,7 +288,7 @@ def side_effect(url, *args, **kwargs): "latestVersion": {"version": "1.0.1"}, }, ) - if url.endswith("/download"): + if "/download" in url: return _MockResponse(status_code=404) if url.endswith("/skills/caldav-calendar/versions/1.0.1"): return _MockResponse( @@ -292,14 +301,18 @@ def side_effect(url, *args, **kwargs): ) return _MockResponse(status_code=404, json_data={}) - mock_get.side_effect = side_effect + mock_safe_get.side_effect = side_effect mock_safe.side_effect = lambda url: not url.startswith("http://127.0.0.1/") bundle = self.src.fetch("caldav-calendar") self.assertIsNone(bundle) - self.assertEqual(mock_get.call_count, 3) - mock_safe_get.assert_not_called() + self.assertEqual(mock_safe_get.call_count, 3) + private_calls = [ + call for call in mock_safe_get.call_args_list + if call.args and str(call.args[0]).startswith("http://127.0.0.1/") + ] + self.assertEqual(private_calls, []) @patch("tools.skills_hub._write_index_cache") @patch("tools.skills_hub._read_index_cache", return_value=None) diff --git a/tests/tools/test_skills_hub_clawhub_ssrf.py b/tests/tools/test_skills_hub_clawhub_ssrf.py new file mode 100644 index 000000000000..a77fb1d5ecd8 --- /dev/null +++ b/tests/tools/test_skills_hub_clawhub_ssrf.py @@ -0,0 +1,44 @@ +"""Regression: ClawHub ZIP download must use SSRF-guarded HTTP.""" + +from unittest.mock import patch + +import httpx + +from tools.skills_hub import ClawHubSource, _guarded_http_get + + +def test_download_zip_uses_guarded_http_get(): + src = ClawHubSource() + with patch("tools.skills_hub._guarded_http_get", return_value=None) as mock_get: + files = src._download_zip("demo-skill", "1.0.0") + assert files == {} + assert mock_get.call_count == 1 + called_url = mock_get.call_args.args[0] + assert "/download" in called_url + assert "slug=demo-skill" in called_url + assert "version=1.0.0" in called_url + + +def test_download_zip_returns_empty_when_ssrf_blocked(): + src = ClawHubSource() + with patch("tools.skills_hub._guarded_http_get", return_value=None): + assert src._download_zip("evil", "9.9.9") == {} + + +def test_guarded_http_get_blocks_private_redirect_before_safe_http_get(): + public_url = "https://clawhub.example/download?slug=demo-skill&version=1.0.0" + private_url = "http://127.0.0.1:8080/admin" + redirect = httpx.Response( + 302, + headers={"location": private_url}, + request=httpx.Request("GET", public_url), + ) + + with ( + patch("tools.skills_hub.is_safe_url", side_effect=lambda url: url == public_url), + patch("tools.skills_hub.check_website_access", return_value=None), + patch("tools.skills_hub._ssrf_safe_http_get", return_value=redirect) as mock_safe_get, + ): + assert _guarded_http_get(public_url) is None + + mock_safe_get.assert_called_once_with(public_url, timeout=20) diff --git a/tools/skills_hub.py b/tools/skills_hub.py index aa1bcc5c01e4..79760b24cbdf 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -2594,11 +2594,11 @@ def _load_catalog_index(self, max_items: int = 0) -> List[SkillMeta]: def _get_json(self, url: str, timeout: int = 20) -> Optional[Any]: try: - resp = httpx.get(url, timeout=timeout) - if resp.status_code != 200: + resp = _guarded_http_get(url, timeout=timeout) + if resp is None or resp.status_code != 200: return None return resp.json() - except (httpx.HTTPError, json.JSONDecodeError): + except (httpx.HTTPError, json.JSONDecodeError, ValueError): return None def _resolve_latest_version(self, slug: str, skill_data: Dict[str, Any]) -> Optional[str]: @@ -2661,14 +2661,19 @@ def _download_zip(self, slug: str, version: str) -> Dict[str, str]: files: Dict[str, str] = {} max_retries = 3 + download_url = f"{self.BASE_URL}/download" + # Build the absolute URL once so guarded redirects can re-validate hops. + from urllib.parse import urlencode + + download_url_with_qs = f"{download_url}?{urlencode({'slug': slug, 'version': version})}" for attempt in range(max_retries): try: - resp = httpx.get( - f"{self.BASE_URL}/download", - params={"slug": slug, "version": version}, - timeout=30, - follow_redirects=True, - ) + # Salvage #57571 (size caps): also block redirect-based SSRF. + # ClawHub CDN may 302; raw follow_redirects=True would allow a + # hop into private/link-local space. + resp = _guarded_http_get(download_url_with_qs, timeout=30) + if resp is None: + return files if resp.status_code == 429: try: retry_after = int(resp.headers.get("retry-after", "5"))