diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 7039d665..bd6868c2 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -26,3 +26,8 @@ **Vulnerability:** Server-Side Request Forgery (SSRF) / Local File Inclusion **Learning:** External URL fetching with `urllib.request.urlopen` (like API endpoints passed via environment variables) can accept schemes like `file://` implicitly, which could allow arbitrary file reading or internal network scanning if the environment is misconfigured or manipulated. **Prevention:** Always validate that URLs explicitly start with `http://` or `https://` before using them in standard library requests. Append to suppress linter warnings only after verifying the input is validated. +## 2026-07-09 - Prevent SSRF via Redirects in urllib + +**Vulnerability:** Initial URL validation for SSRF (e.g., checking scheme and IP address) is insufficient if the HTTP client automatically follows redirects. In `urllib.request.urlopen`, redirects are followed by default, allowing an attacker to bypass initial checks by returning a 302 redirect to an internal IP (like `169.254.169.254` or `127.0.0.1`). +**Learning:** `urllib.request.urlopen` does not inherit the security properties of the initial URL string check. It will follow HTTP redirects unconditionally to any target URL, creating a severe SSRF risk when dealing with external API endpoints that can be manipulated by malicious responses. +**Prevention:** Explicitly disable redirects by subclassing `urllib.request.HTTPRedirectHandler`, overriding `redirect_request` to raise an `urllib.error.HTTPError`, and using `urllib.request.build_opener(NoRedirectHandler())` instead of the default `urlopen`. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 46f3c103..cde5fd82 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -11,6 +11,7 @@ import socket import subprocess import sys +import urllib.error import urllib.parse import urllib.request from collections.abc import Sequence @@ -257,6 +258,22 @@ def fetch_diff(repo: str, number: int) -> tuple[str, bool]: return diff, truncated +class NoRedirectHandler(urllib.request.HTTPRedirectHandler): + """A URL opener handler that refuses to follow redirects to prevent SSRF.""" + + def redirect_request( + self, + req: urllib.request.Request, + fp: Any, + code: int, + msg: str, + headers: Any, + newurl: str, + ) -> None: + """Raise an HTTPError instead of following the redirect.""" + raise urllib.error.HTTPError(req.full_url, code, msg, headers, fp) + + def extract_json_object(text: str) -> dict[str, Any]: """Extract a JSON object from a strict or lightly wrapped LLM response.""" stripped = text.strip() @@ -341,7 +358,8 @@ def call_llm(repo: str, number: int, pr: dict[str, Any], diff: str, truncated: b }, method="POST", ) - with urllib.request.urlopen(request, timeout=120) as response: # nosec B310 + opener = urllib.request.build_opener(NoRedirectHandler()) + with opener.open(request, timeout=120) as response: # nosec B310 raw = response.read().decode("utf-8") data = json.loads(raw) content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index a4fb4a05..3a1aecaa 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -216,23 +216,33 @@ def fake_urlopen(request, timeout): seen["body"] = json.loads(request.data.decode("utf-8")) return FakeResponse({"choices": [{"message": {"content": '{"decision":"approve","summary":"ok","findings":[]}'}}]}) - monkeypatch.setattr(noema.urllib.request, "urlopen", fake_urlopen) + # Since we replaced urlopen with build_opener, we mock build_opener + class FakeOpener: + def __init__(self, call_func): + self.call_func = call_func + def open(self, request, timeout=None): + return self.call_func(request, timeout) + + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) verdict = noema.call_llm("owner/repo", 1, pr, "diff", True) assert verdict["decision"] == "approve" assert seen["url"] == "https://llm.example.test/chat" assert seen["body"]["model"] == "review-model" + def fake_urlopen_defer(request, timeout=None): + return FakeResponse({"choices": [{"message": {"content": '{"decision":"defer"}'}}]}) + monkeypatch.setattr( noema.urllib.request, - "urlopen", - lambda *args, **kwargs: FakeResponse({"choices": [{"message": {"content": '{"decision":"defer"}'}}]}), + "build_opener", + lambda *args: FakeOpener(fake_urlopen_defer) ) with pytest.raises(RuntimeError, match="unsupported decision"): noema.call_llm("owner/repo", 1, pr, "diff", False) # Test case-insensitive valid URL monkeypatch.setenv("NOEMA_LLM_API_URL", "HTTPS://llm.example.test/chat") - monkeypatch.setattr(noema.urllib.request, "urlopen", fake_urlopen) + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" # Test invalid scheme (and no original URL in error) @@ -273,7 +283,7 @@ def fake_getaddrinfo(host, port, *args, **kwargs): def fake_getaddrinfo_error(host, port, *args, **kwargs): raise socket.gaierror("Name or service not known") monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo_error) - monkeypatch.setattr(noema.urllib.request, "urlopen", fake_urlopen) + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" # Test invalid IP string from getaddrinfo (unlikely but theoretically possible) @@ -286,6 +296,22 @@ def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" +def test_noema_redirect_handler_rejects_redirects(): + """Noema must not follow redirects after validating the initial URL.""" + handler = noema.NoRedirectHandler() + request = noema.urllib.request.Request("https://llm.example.test/chat") + + with pytest.raises(noema.urllib.error.HTTPError): + handler.redirect_request( + request, + fp=None, + code=302, + msg="Found", + headers={}, + newurl="http://169.254.169.254/latest/meta-data/", + ) + + def test_call_llm_rejects_control_character_scheme_evasion(monkeypatch): """A URL with an embedded tab is normalized by urlparse to an http scheme with a valid hostname, but its raw form does not start with http:// — the