diff --git a/services/memory/skill_importer.py b/services/memory/skill_importer.py index 65f4b21c0..2f0d7ab32 100644 --- a/services/memory/skill_importer.py +++ b/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,22 +122,18 @@ 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) - 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) - _assert_github_url(final, context="redirect target") - # Page may embed a github link; prefer final URL if redirected. - if "github.com" in final: - raw = final - else: - m = re.search(r"https?://github\.com/[^\s\"')]+", r.text or "") - if m: - raw = m.group(0).rstrip(".,)") + r = _get_checked(raw, timeout=20.0) + if r.status_code >= 400: + raise _github_response_error(r) + final = str(r.url) + _assert_github_url(final, context="redirect target") + # Page may embed a github link; prefer final URL if redirected. + if "github.com" in final: + raw = final + else: + m = re.search(r"https?://github\.com/[^\s\"')]+", r.text or "") + if m: + raw = m.group(0).rstrip(".,)") parsed = urlparse(raw) host = _github_host(raw) @@ -165,17 +207,13 @@ def _github_response_error(response: httpx.Response) -> SkillImportError: def _fetch_bytes(url: str) -> bytes: - ok, reason = check_outbound_url(url) - 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") - if len(r.content) > MAX_FILE_BYTES: - raise SkillImportError(f"file too large: {url}") - return r.content + r = _get_checked(url, headers={"Accept": "application/vnd.github+json"}, timeout=30.0) + if r.status_code >= 400: + raise _github_response_error(r) + _assert_github_url(str(r.url), context="redirect target") + if len(r.content) > MAX_FILE_BYTES: + raise SkillImportError(f"file too large: {url}") + return r.content def _fetch_text(url: str) -> str: @@ -190,15 +228,11 @@ 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) - 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") - entries = r.json() + r = _get_checked(url, headers={"Accept": "application/vnd.github+json"}, timeout=30.0) + if r.status_code >= 400: + raise _github_response_error(r) + _assert_github_url(str(r.url), context="redirect target") + entries = r.json() if not isinstance(entries, list): raise SkillImportError("expected a directory on GitHub") total = sum(len(v.encode("utf-8")) for v in out.values()) diff --git a/tests/test_skill_importer.py b/tests/test_skill_importer.py index eecca614f..d7822711b 100644 --- a/tests/test_skill_importer.py +++ b/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, ""), ) diff --git a/tests/test_skill_importer_ssrf_redirect.py b/tests/test_skill_importer_ssrf_redirect.py new file mode 100644 index 000000000..800be633f --- /dev/null +++ b/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