fix(skills): block private SSRF targets and revalidate redirects in importer (#5261)

* Harden skill importer against SSRF: block private targets + revalidate redirects per hop

The skill importer validated only the initial URL with the lenient SSRF guard
(block_private=False) and then fetched with follow_redirects=True, so a 3xx to
an internal/metadata address (169.254.169.254, 127.0.0.1, RFC-1918) was still
connected to — inconsistent with the hardened services/search/content.py
:_get_public_url path.

Add a _get_checked() helper that follows redirects manually and re-runs the
SSRF guard with block_private=True on every hop, and route all three fetch
sites (skills.sh unwrap, _fetch_bytes, _list_github_dir) through it. GitHub's
own redirects and the final-host _assert_github_url checks are preserved.

Adds hermetic regression tests (IP-literal hosts, faked HTTP layer) and updates
the existing mock signature for the new block_private kwarg.

Defense-in-depth: the endpoint is admin-gated (require_admin) and admins are
trusted per THREAT_MODEL.md, so this is not a cross-boundary vulnerability.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: enforce follow_redirects=False invariant in mock client

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

services/memory/skill_importer.py

@@ -6,7 +6,7 @@ import os
 import re
 from dataclasses import dataclass
 from typing import Dict, List, Optional, Tuple
-from urllib.parse import quote, urlparse
+from urllib.parse import quote, urljoin, urlparse
 
 import httpx
 
@@ -68,6 +68,52 @@ def _is_text_file(name: str) -> bool:
     return any(low.endswith(s) for s in ALLOWED_SUFFIXES)
 
 
+# Max redirect hops to follow manually while re-validating each one.
+_MAX_FETCH_REDIRECTS = 5
+
+
+def _check_fetch_url(url: str) -> None:
+    """SSRF guard for skill-import fetches (defense-in-depth).
+
+    Skill bundles only ever come from public GitHub, never an internal
+    address, so block private/loopback/link-local targets on every hop —
+    matching the hardened web-fetch path in
+    ``services/search/content.py:_get_public_url`` rather than the lenient
+    default used for admin-configured model endpoints.
+    """
+    ok, reason = check_outbound_url(url, block_private=True)
+    if not ok:
+        raise SkillImportError(reason)
+
+
+def _get_checked(
+    url: str,
+    *,
+    headers: Optional[dict] = None,
+    timeout: float = 30.0,
+) -> httpx.Response:
+    """GET that follows redirects manually, re-running the SSRF guard per hop.
+
+    ``httpx``'s ``follow_redirects=True`` validates only the initial URL, so a
+    ``3xx`` to an internal address (``169.254.169.254``, ``127.0.0.1``, …) would
+    still be connected to before any post-hoc host check. Following redirects by
+    hand lets us re-validate every hop, closing that blind-SSRF gap.
+    """
+    current = url
+    with httpx.Client(follow_redirects=False, timeout=timeout) as client:
+        for _ in range(_MAX_FETCH_REDIRECTS + 1):
+            _check_fetch_url(current)
+            r = client.get(current, headers=headers)
+            if r.status_code in (301, 302, 303, 307, 308):
+                location = r.headers.get("location")
+                if not location:
+                    return r
+                current = urljoin(str(r.url), location)
+                continue
+            return r
+    raise SkillImportError("too many redirects while fetching skill bundle")
+
+
 def parse_skill_source(url: str) -> ResolvedSource:
     """Normalize skills.sh / GitHub web URLs into owner/repo/ref/path."""
     raw = (url or "").strip()
@@ -76,11 +122,7 @@ def parse_skill_source(url: str) -> ResolvedSource:
 
     # skills.sh often links to GitHub; try to unwrap ?url= or redirect target later.
     if "skills.sh" in raw and "github.com" not in raw:
-        ok, reason = check_outbound_url(raw)
+        r = _get_checked(raw, timeout=20.0)
-        if not ok:
-            raise SkillImportError(reason)
-        with httpx.Client(follow_redirects=True, timeout=20.0) as client:
-            r = client.get(raw)
         if r.status_code >= 400:
             raise _github_response_error(r)
         final = str(r.url)
@@ -165,11 +207,7 @@ def _github_response_error(response: httpx.Response) -> SkillImportError:
 
 
 def _fetch_bytes(url: str) -> bytes:
-    ok, reason = check_outbound_url(url)
+    r = _get_checked(url, headers={"Accept": "application/vnd.github+json"}, timeout=30.0)
-    if not ok:
-        raise SkillImportError(reason)
-    with httpx.Client(follow_redirects=True, timeout=30.0) as client:
-        r = client.get(url, headers={"Accept": "application/vnd.github+json"})
     if r.status_code >= 400:
         raise _github_response_error(r)
     _assert_github_url(str(r.url), context="redirect target")
@@ -190,11 +228,7 @@ def _list_github_dir(src: ResolvedSource, rel_dir: str, out: Dict[str, str], *,
     if depth > 4 or len(out) >= MAX_FILES:
         return
     url = _api_contents_url(src, rel_dir)
-    ok, reason = check_outbound_url(url)
+    r = _get_checked(url, headers={"Accept": "application/vnd.github+json"}, timeout=30.0)
-    if not ok:
-        raise SkillImportError(reason)
-    with httpx.Client(follow_redirects=True, timeout=30.0) as client:
-        r = client.get(url, headers={"Accept": "application/vnd.github+json"})
     if r.status_code >= 400:
         raise _github_response_error(r)
     _assert_github_url(str(r.url), context="redirect target")

tests/test_skill_importer.py

@@ -71,7 +71,7 @@ def test_fetch_bytes_rejects_cross_host_redirect(monkeypatch):
     monkeypatch.setattr("services.memory.skill_importer.httpx.Client", _Client)
     monkeypatch.setattr(
         "services.memory.skill_importer.check_outbound_url",
-        lambda url: (True, ""),
+        lambda url, **kwargs: (True, ""),
     )
     with pytest.raises(SkillImportError, match="redirect target"):
         _fetch_bytes("https://raw.githubusercontent.com/o/r/main/SKILL.md")
@@ -91,7 +91,7 @@ def test_list_github_dir_accepts_api_github_response(monkeypatch):
     )
     monkeypatch.setattr(
         "services.memory.skill_importer.check_outbound_url",
-        lambda url: (True, ""),
+        lambda url, **kwargs: (True, ""),
     )
 
     class _Resp:
@@ -146,7 +146,7 @@ def _mock_httpx_client(monkeypatch, response):
     monkeypatch.setattr("services.memory.skill_importer.httpx.Client", _Client)
     monkeypatch.setattr(
         "services.memory.skill_importer.check_outbound_url",
-        lambda url: (True, ""),
+        lambda url, **kwargs: (True, ""),
     )
 
 

tests/test_skill_importer_ssrf_redirect.py

@@ -0,0 +1,123 @@
+"""Skill importer SSRF hardening: redirects must be re-validated per hop.
+
+The importer follows redirects manually (`_get_checked`) and re-runs the SSRF
+guard on every hop with ``block_private=True``, matching the hardened web-fetch
+path in ``services/search/content.py:_get_public_url``. Previously it used
+``httpx``'s ``follow_redirects=True`` with the lenient guard on the *initial*
+URL only, so a ``3xx`` to an internal/metadata address was still connected to.
+
+These tests are hermetic: every host is an IP literal, so ``check_outbound_url``
+resolves them locally (``getaddrinfo`` on a numeric address does no DNS) and no
+network access is required. The HTTP layer is faked so no real request is made.
+"""
+import pytest
+
+from services.memory import skill_importer
+from services.memory.skill_importer import (
+    SkillImportError,
+    _check_fetch_url,
+    _fetch_bytes,
+    _get_checked,
+    parse_skill_source,
+)
+
+# Clearly-public, non-reserved IP literals for the initial (allowed) hop.
+PUBLIC_A = "https://1.1.1.1/skill"
+PUBLIC_B = "https://8.8.8.8/skill"
+# Internal redirect targets that must be refused before connection.
+LOOPBACK = "http://127.0.0.1/latest"
+METADATA = "http://169.254.169.254/latest/meta-data/"
+
+
+def _install_fake_client(monkeypatch, *, redirect_from, redirect_to):
+    """Replace httpx.Client so `redirect_from` 302s to `redirect_to`, and any
+    other URL returns 200. No real socket is opened."""
+
+    class _Resp:
+        def __init__(self, url, status, location):
+            self.url = url
+            self.status_code = status
+            self.headers = {"location": location} if location else {}
+            self.content = b"ok"
+            self.text = ""
+
+        def raise_for_status(self):
+            return None
+
+        def json(self):
+            return {}
+
+    class _Client:
+        def __init__(self, *args, **kwargs):
+            # Safety invariant: the importer follows redirects by hand and
+            # re-runs the SSRF guard per hop, so it MUST disable httpx's own
+            # redirect following. Asserting ``follow_redirects is False`` here
+            # (not merely accepting the kwarg) makes any regression to
+            # ``follow_redirects=True`` fail these tests instead of passing
+            # silently — httpx being faked would otherwise hide the change.
+            assert kwargs.get("follow_redirects") is False, (
+                "skill importer must construct httpx.Client with "
+                "follow_redirects=False; got "
+                f"{kwargs.get('follow_redirects')!r}"
+            )
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, *args):
+            return False
+
+        def get(self, url, headers=None):
+            if url == redirect_from:
+                return _Resp(url, 302, redirect_to)
+            return _Resp(url, 200, None)
+
+    monkeypatch.setattr(skill_importer.httpx, "Client", _Client)
+
+
+# --- Guard unit: block_private=True refuses internal, allows public ----------
+
+@pytest.mark.parametrize("url", [LOOPBACK, METADATA, "http://10.0.0.5/", "http://[::1]/"])
+def test_check_fetch_url_blocks_internal(url):
+    with pytest.raises(SkillImportError):
+        _check_fetch_url(url)
+
+
+@pytest.mark.parametrize("url", [PUBLIC_A, PUBLIC_B])
+def test_check_fetch_url_allows_public(url):
+    # Should not raise for a public IP literal.
+    _check_fetch_url(url)
+
+
+# --- Redirect revalidation: the core regression ------------------------------
+
+@pytest.mark.parametrize("internal", [LOOPBACK, METADATA])
+def test_get_checked_blocks_redirect_to_internal(monkeypatch, internal):
+    _install_fake_client(monkeypatch, redirect_from=PUBLIC_A, redirect_to=internal)
+    with pytest.raises(SkillImportError, match="blocked"):
+        _get_checked(PUBLIC_A)
+
+
+@pytest.mark.parametrize("internal", [LOOPBACK, METADATA])
+def test_fetch_bytes_blocks_redirect_to_internal(monkeypatch, internal):
+    # Higher-level: the public fetch helpers inherit the per-hop guard.
+    _install_fake_client(monkeypatch, redirect_from=PUBLIC_A, redirect_to=internal)
+    with pytest.raises(SkillImportError, match="blocked"):
+        _fetch_bytes(PUBLIC_A)
+
+
+def test_skills_sh_entry_blocks_redirect_to_metadata(monkeypatch):
+    # The skills.sh unwrap path (user-supplied host) must also revalidate hops.
+    raw = "http://1.1.1.1/skills.sh"  # contains "skills.sh", not "github.com"
+    _install_fake_client(monkeypatch, redirect_from=raw, redirect_to=METADATA)
+    with pytest.raises(SkillImportError, match="blocked"):
+        parse_skill_source(raw)
+
+
+# --- Positive: a legitimate public->public redirect is still followed --------
+
+def test_get_checked_follows_public_redirect(monkeypatch):
+    _install_fake_client(monkeypatch, redirect_from=PUBLIC_A, redirect_to=PUBLIC_B)
+    resp = _get_checked(PUBLIC_A)
+    assert resp.status_code == 200
+    assert str(resp.url) == PUBLIC_B