From b00e8ef44222ae407b3e8a07a73ea27f9fdb508b Mon Sep 17 00:00:00 2001
From: Bastien Chanot <git@bchanot.fr>
Date: Fri, 17 Jul 2026 19:58:59 +0200
Subject: [PATCH] =?UTF-8?q?feat(seo-data):=20safe=5Ffetch=20=E2=80=94=20re?=
 =?UTF-8?q?solve-then-pin,=20close=20DNS-rebinding=20+=20SSRF?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

By-principle hardening. H1's url-guard validates the NAME; urlopen then resolved
AND connected — two DNS lookups with a window a hostile authority uses to answer
PUBLIC to validation and PRIVATE (169.254.169.254 metadata, 127.0.0.1, the LAN)
to the connect. A name-level guard cannot see that rebind.

safe_fetch collapses the two lookups into one: resolve ONCE, validate every IP
(ipaddress, dual-stack v4+v6), refuse if ANY is non-public (the multi-A vector),
connect to the exact validated IP with Host+SNI+cert for the real host — no
second resolution to poison. Redirects re-validate each hop (urlopen followed
them blind). One seam: sitemap._fetch, which linkgraph/render_check/drift all
call, so every network verb inherits it.

The load-bearing property (confirmed by the security review): classification is
on the OS-resolved address (sockaddr[0]), never the URL text — so octal/hex/
decimal literals, IPv4-mapped IPv6, NAT64, 6to4 are all defeated structurally,
not by enumeration.

Better than the source idea (claude-seo url_safety.py, MIT): dual-stack (theirs
IPv4-only), no global monkeypatch so thread-safe by construction (theirs locks a
patched getaddrinfo), stdlib-only (no requests). Proven end-to-end before
writing: pinned connect keeps SNI+cert for the real host.

NOT covered, stated not silent: shell `curl` in the agent specs (separate
process, unpinnable here). Smaller surface; `curl --resolve` is a separate change.

REVIEW-SURFACED (fresh security-auditor, adversarial, VERDICT PASS) — two real
holes it found while attacking the diff, both fixed here:
- billion-laughs REOPENED in C1b: _refuse_dtd scanned only raw[:4096], so a
  >4KB leading comment pushed <!DOCTYPE past the window while ET parsed AND
  EXPANDED the entities. Proven (&lol2; → "lollollollollol"), now a full-doc
  case-insensitive scan. This is a genuine fix to already-merged C1b, not this
  feature — fixed here rather than filed, per root-cause discipline.
- 192.88.99.0/24 (6to4-relay anycast) passed is_global as public — added to an
  extra special-use deny list.

Verified: rebind-to-metadata refused BEFORE any connect (injected resolver),
multi-A public+private refused, classifier fuzzed dual-stack incl. CGNAT/6to4,
non-http scheme refused, both review fixes proven with no false positive; real
fetch still works (zenquality 86 loc, lavageangels 24) through the pinned path;
all 4 verbs work end-to-end via fetch.sh; seo-data 210 → 221 pass, 0 fail; full
suite green; shellcheck + py_compile clean.
---
 lib/seo-data/README.md        |  17 ++++
 lib/seo-data/safe_fetch.py    | 164 ++++++++++++++++++++++++++++++++++
 lib/seo-data/seo-data.test.sh |  51 +++++++++++
 lib/seo-data/sitemap.py       |  19 ++--
 lib/url-guard.sh              |  12 +--
 5 files changed, 252 insertions(+), 11 deletions(-)
 create mode 100644 lib/seo-data/safe_fetch.py

diff --git a/lib/seo-data/README.md b/lib/seo-data/README.md
index 1f277c5..55a9086 100644
--- a/lib/seo-data/README.md
+++ b/lib/seo-data/README.md
@@ -120,6 +120,23 @@ fetch.sh cannibal --account client-a --property … [--days 90] [--rows 1000]
       Rows gained a `keys` list; `key` stays as keys[0], so the single-dim
       consumer is untouched.
 
+safe_fetch.py — NOT a verb; the SSRF/DNS-rebinding-safe fetcher behind
+  sitemap._fetch, so every network verb (sitemap, linkgraph, rendercheck,
+  drift) inherits it. urlopen resolved then connected — two DNS lookups, a
+  window a hostile authority uses to answer PUBLIC to validation and PRIVATE
+  (169.254.169.254 metadata, 127.0.0.1, the LAN) to the connect. This resolves
+  ONCE, validates every IP (ipaddress, dual-stack v4+v6), refuses if ANY is
+  non-public (the multi-A vector), and connects to the exact validated IP with
+  Host+SNI+cert for the real host — no second resolution to poison. Redirects
+  are followed with each hop RE-VALIDATED (urlopen followed them blind).
+    • Better than the source idea (claude-seo url_safety.py, MIT): dual-stack
+      (theirs IPv4-only), no global monkeypatch so thread-safe by construction
+      (theirs locks a patched socket.getaddrinfo), stdlib-only (no requests).
+    • Refusal raises UnsafeTarget; callers already degrade → fail-open kept.
+    • NOT covered, and said so: the shell `curl` in the agent specs runs in
+      another process, unpinnable from here. Smaller surface (fixed set vs an
+      operator-confirmed $DOMAIN); `curl --resolve` would close it, separate change.
+
 fetch.sh sitemap --url https://ex.com/sitemap.xml
   → {"status":"ok","source":"sitemap","index":false,"count":86,"dropped":0,
      "urls":["https://ex.com/", …]}
diff --git a/lib/seo-data/safe_fetch.py b/lib/seo-data/safe_fetch.py
new file mode 100644
index 0000000..820abd8
--- /dev/null
+++ b/lib/seo-data/safe_fetch.py
@@ -0,0 +1,164 @@
+#!/usr/bin/env python3
+"""SSRF- and DNS-rebinding-safe HTTP(S) fetch. Stdlib only.
+
+The verbs that fetch remote content (sitemap, linkgraph, render_check, drift)
+all route through sitemap._fetch, which used urllib.request.urlopen. urlopen
+resolves the host, then connects — two DNS lookups with a window between them.
+A hostile authority can answer PUBLIC to the validation lookup and a PRIVATE
+address (169.254.169.254 cloud metadata, 127.0.0.1, the LAN) to the connect
+lookup. That is DNS rebinding, and a name-level guard cannot see it.
+
+This collapses the two lookups into one: resolve ONCE, validate every returned
+IP, then connect to the exact validated IP while preserving the Host header,
+TLS SNI, and certificate validation for the real hostname. There is no second
+resolution to poison.
+
+Better than the reference implementation this idea came from (claude-seo
+url_safety.py, MIT) on three axes, all verified before writing:
+- dual-stack: validates IPv4 AND IPv6 (theirs is IPv4-only);
+- no global state: each connection pins its own socket, so it is thread-safe
+  by construction (theirs monkeypatches socket.getaddrinfo behind a global
+  lock);
+- stdlib only: http.client + ssl + ipaddress, no `requests`.
+
+NOT covered, stated rather than left silent: the shell `curl` calls in the
+agent specs (seo-analyzer/geo-analyzer STEP 4, the sameAs loop) run in a
+separate process and cannot be pinned from here. Their surface is smaller
+(a fixed set against an operator-typed/confirmed $DOMAIN). Closing them needs
+`curl --resolve` and is a separate change.
+"""
+import gzip
+import http.client
+import ipaddress
+import socket
+import ssl
+from urllib.parse import urljoin, urlparse
+
+DEFAULT_TIMEOUT = 20
+DEFAULT_MAX_BYTES = 20 * 1024 * 1024
+MAX_REDIRECTS = 5
+
+
+class UnsafeTarget(Exception):
+    """A URL resolved to a non-public address, or a redirect did. Raised BEFORE
+    any connection to that address. Callers already wrap _fetch in try/except
+    and degrade, so the fail-open contract is preserved."""
+
+
+# Special-use ranges that `is_global` reports as public but are not legitimate
+# fetch targets. 192.88.99.0/24 = RFC 3068 6to4-relay anycast (a security
+# review flagged it 2026-07-17). Grows if more surface.
+_EXTRA_DENY = (ipaddress.ip_network("192.88.99.0/24"),)
+
+
+def _ip_is_public(ip_str):
+    """A globally routable unicast address, dual-stack. `is_global` is the
+    decisive gate — it alone rejects CGNAT (100.64/10) that the per-flag checks
+    miss — with the explicit flags plus an extra special-use deny list as
+    defence in depth."""
+    ip = ipaddress.ip_address(ip_str)
+    if not ip.is_global:
+        return False
+    if any(ip in net for net in _EXTRA_DENY):
+        return False
+    return not (ip.is_private or ip.is_loopback or ip.is_link_local
+                or ip.is_reserved or ip.is_multicast or ip.is_unspecified)
+
+
+def _resolve_pinned(host, port, resolver=socket.getaddrinfo):
+    """Resolve host ONCE and return [(family, ip)] for connecting. Refuse if
+    ANY resolved address is non-public — a name advertising both public and
+    private A records is exactly the multi-answer rebinding vector, and a
+    legitimate public site does not do it. `resolver` is injected in tests to
+    plant a private address and prove the refusal."""
+    try:
+        infos = resolver(host, port, type=socket.SOCK_STREAM)
+    except socket.gaierror as e:
+        raise UnsafeTarget("cannot resolve %r: %s" % (host, e))
+    pinned = []
+    for family, _type, _proto, _canon, sockaddr in infos:
+        ip = sockaddr[0]
+        if not _ip_is_public(ip):
+            raise UnsafeTarget("%s resolves to non-public %s" % (host, ip))
+        pinned.append((family, ip))
+    if not pinned:
+        raise UnsafeTarget("%s resolved to nothing" % host)
+    return pinned
+
+
+class _PinnedHTTPSConnection(http.client.HTTPSConnection):
+    """HTTPS to a pinned IP, with SNI + cert validation for the real host."""
+    def __init__(self, host, pinned_ip, family, **kw):
+        super().__init__(host, **kw)          # host → Host header + SNI
+        self._pinned_ip = pinned_ip
+        self._family = family
+
+    def connect(self):
+        sock = socket.create_connection((self._pinned_ip, self.port),
+                                        timeout=self.timeout)
+        # server_hostname = the real host → SNI + hostname check both use it,
+        # never the IP.
+        self.sock = self._context.wrap_socket(sock, server_hostname=self.host)
+
+
+class _PinnedHTTPConnection(http.client.HTTPConnection):
+    """Plain HTTP to a pinned IP (Host header stays the real host)."""
+    def __init__(self, host, pinned_ip, family, **kw):
+        super().__init__(host, **kw)
+        self._pinned_ip = pinned_ip
+        self._family = family
+
+    def connect(self):
+        self.sock = socket.create_connection((self._pinned_ip, self.port),
+                                             timeout=self.timeout)
+
+
+def _one_request(url, timeout, max_bytes, resolver):
+    """One hop: resolve+pin the host, connect, return (status, headers, body)."""
+    p = urlparse(url)
+    if p.scheme not in ("http", "https"):
+        raise UnsafeTarget("scheme must be http/https: %r" % url)
+    host = p.hostname
+    if not host:
+        raise UnsafeTarget("no host in %r" % url)
+    port = p.port or (443 if p.scheme == "https" else 80)
+    family, ip = _resolve_pinned(host, port, resolver)[0]   # any is public here
+    ctx = ssl.create_default_context() if p.scheme == "https" else None
+    if p.scheme == "https":
+        conn = _PinnedHTTPSConnection(host, ip, family, port=port,
+                                      timeout=timeout, context=ctx)
+    else:
+        conn = _PinnedHTTPConnection(host, ip, family, port=port,
+                                     timeout=timeout)
+    try:
+        path = p.path or "/"
+        if p.query:
+            path += "?" + p.query
+        # No Accept-Encoding: keep HTTP bodies un-gzipped; the .xml.gz
+        # content-level case is handled by the caller's magic-byte check.
+        conn.request("GET", path, headers={"Host": host,
+                                           "User-Agent": "claude-seo-data/1.0"})
+        r = conn.getresponse()
+        body = r.read(max_bytes)
+        return r.status, {k.lower(): v for k, v in r.getheaders()}, body
+    finally:
+        conn.close()
+
+
+def safe_fetch(url, timeout=DEFAULT_TIMEOUT, max_bytes=DEFAULT_MAX_BYTES,
+               max_redirects=MAX_REDIRECTS, resolver=socket.getaddrinfo):
+    """Fetch url with resolve-then-pin, following redirects and RE-VALIDATING
+    each hop — urlopen followed redirects to whatever address the Location
+    named, re-opening the rebinding window on every hop. Returns the raw body
+    bytes (the caller handles content-level gzip)."""
+    seen = 0
+    current = url
+    while True:
+        status, headers, body = _one_request(current, timeout, max_bytes, resolver)
+        if status in (301, 302, 303, 307, 308) and "location" in headers:
+            seen += 1
+            if seen > max_redirects:
+                raise UnsafeTarget("too many redirects from %r" % url)
+            current = urljoin(current, headers["location"])   # re-validated next loop
+            continue
+        return body
diff --git a/lib/seo-data/seo-data.test.sh b/lib/seo-data/seo-data.test.sh
index fe8012a..d13d260 100644
--- a/lib/seo-data/seo-data.test.sh
+++ b/lib/seo-data/seo-data.test.sh
@@ -99,6 +99,47 @@ check_first() { [ "$1" = "$2" ] && ok "$3" || no "$3" "got[$1]"; }
 check_first "$CAN_FIRST" "urgence fuite https://ex.com/urgence" "cannibal ranks by impact"
 has "cannibal reports the cap"    "$CAN" '"capped": false'
 
+echo "── safe_fetch (DNS-rebinding / SSRF) ──"
+# Inject a hostile resolver: the name is public, the address is internal. This
+# is the rebinding vector a name-level guard cannot see — prove it is refused
+# BEFORE any connection. Deterministic + offline via the injected resolver.
+sfpy() { PYTHONPATH="$SD" python3 -c "$1" 2>&1; }
+REBIND="$(sfpy '
+import socket, safe_fetch as sf
+def meta(h,p,**k): return [(socket.AF_INET,socket.SOCK_STREAM,6,"",("169.254.169.254",p))]
+try: sf.safe_fetch("https://evil.example/", resolver=meta); print("CONNECTED")
+except sf.UnsafeTarget as e: print("REFUSED", e)')"
+has "rebind to metadata refused"  "$REBIND" 'REFUSED'
+has "refusal names the ip"        "$REBIND" '169.254.169.254'
+hasnt "never connected"           "$REBIND" 'CONNECTED'
+MIXED="$(sfpy '
+import socket, safe_fetch as sf
+def mix(h,p,**k): return [(socket.AF_INET,socket.SOCK_STREAM,6,"",("93.184.216.34",p)),
+                          (socket.AF_INET,socket.SOCK_STREAM,6,"",("127.0.0.1",p))]
+try: sf.safe_fetch("https://evil.example/", resolver=mix); print("CONNECTED")
+except sf.UnsafeTarget as e: print("REFUSED")')"
+has "multi-A public+private refused" "$MIXED" 'REFUSED'
+# classification, dual-stack — is_global catches CGNAT the per-flags miss
+CLS="$(sfpy '
+import safe_fetch as sf
+pub=[c for c in ["8.8.8.8","2606:2800:220:1:248:1893:25c8:1946"] if sf._ip_is_public(c)]
+bad=[c for c in ["169.254.169.254","127.0.0.1","10.0.0.1","192.168.1.1","100.64.1.1","::1","fe80::1","0.0.0.0"] if sf._ip_is_public(c)]
+print("PUB",len(pub),"BADPASS",len(bad))')"
+has "public v4+v6 pass"           "$CLS" 'PUB 2'
+has "no internal ip passes"       "$CLS" 'BADPASS 0'
+# security review 2026-07-17: 6to4-relay anycast passes is_global — extra deny
+SIXTOFOUR="$(sfpy 'import safe_fetch as sf; print("6TO4", sf._ip_is_public("192.88.99.1"))')"
+has "6to4 relay anycast refused"  "$SIXTOFOUR" '6TO4 False'
+# scheme + stdlib
+SCHEME="$(sfpy '
+import safe_fetch as sf
+try: sf.safe_fetch("file:///etc/passwd"); print("OK")
+except sf.UnsafeTarget: print("REFUSED")')"
+has "non-http scheme refused"     "$SCHEME" 'REFUSED'
+IMP="$(/bin/grep -E "^(import|from) " "$SD/safe_fetch.py" | /bin/grep -cvE "gzip|http\.client|ipaddress|socket|ssl|urllib\.parse")"
+[ "$IMP" = "0" ] && ok "safe_fetch is stdlib-only" || no "safe_fetch is stdlib-only" "$IMP non-stdlib imports"
+hasnt "no requests dependency"    "$(cat "$SD/safe_fetch.py")" 'import requests'
+
 echo "── sitemap ──"
 SM="$(SEO_DATA_MOCK_DIR="$MOCK" python3 "$SD/sitemap.py" --url https://ex.com/sitemap.xml)"
 has "sitemap ok"                 "$SM" '"status": "ok"'
@@ -134,6 +175,16 @@ DTD="$(SEO_DATA_MOCK_DIR="$SD/fixtures-sitemap-dtd" python3 "$SD/sitemap.py" \
 has "billion-laughs refused"     "$DTD" '"status": "degraded"'
 has "dtd reason is distinct"     "$DTD" 'unsafe_xml_dtd'
 hasnt "dtd never parsed"         "$DTD" '"count"'
+# security review 2026-07-17: a >4KB leading comment pushed <!DOCTYPE past the
+# old raw[:4096] scan while ET still parsed+expanded it. Now the whole doc is
+# scanned. Prove a padded DTD is refused and the entity never expands.
+PADDED="$(python3 -c '
+import sys; sys.path.insert(0,"'"$SD"'"); import sitemap as sm
+bomb=("<?xml version=\"1.0\"?>\n<!-- "+("x"*5000)+" -->\n"
+      "<!DOCTYPE d [ <!ENTITY lol \"lol\"> ]>\n<urlset><url><loc>https://x/&lol;</loc></url></urlset>").encode()
+try: sm._refuse_dtd(bomb); print("PARSED")
+except sm.UnsafeXML: print("REFUSED")')"
+has "padded DTD refused (full scan)" "$PADDED" 'REFUSED'
 
 echo "── render_check (R2) ──"
 SPA="$(SEO_DATA_MOCK_DIR="$SD/fixtures-spa" python3 "$SD/render_check.py" \
diff --git a/lib/seo-data/sitemap.py b/lib/seo-data/sitemap.py
index 99454ab..5eb3afa 100644
--- a/lib/seo-data/sitemap.py
+++ b/lib/seo-data/sitemap.py
@@ -29,10 +29,11 @@ def _mock(name):
         return f.read()
 
 def _fetch(url):
-    from urllib.request import urlopen, Request      # stdlib, lazy
-    req = Request(url, headers={"User-Agent": "claude-seo-data/1.0"})
-    with urlopen(req, timeout=TIMEOUT) as r:         # nosec: audited target
-        raw = r.read(20 * 1024 * 1024)               # 20 MB ceiling
+    # SSRF + DNS-rebinding safe: resolve-then-pin, redirects re-validated.
+    # This is the single seam for ALL network egress — linkgraph/render_check/
+    # drift all call sitemap._fetch — so pinning here covers every verb.
+    import safe_fetch                                # sibling, lazy
+    raw = safe_fetch.safe_fetch(url, timeout=TIMEOUT, max_bytes=20 * 1024 * 1024)
     if raw[:2] == b"\x1f\x8b":                       # sitemap.xml.gz is common
         raw = gzip.decompress(raw)
     return raw
@@ -53,8 +54,14 @@ def _refuse_dtd(raw):
     google_seo.py's mock/degrade paths. A sitemap with a DTD is not a sitemap
     we want anyway.
     """
-    head = raw[:4096].lstrip()[:2048].upper()
-    if b"<!DOCTYPE" in head or b"<!ENTITY" in head:
+    # Scan the WHOLE document, not a prefix. A security review (2026-07-17)
+    # showed a >4 KB leading comment pushed <!DOCTYPE past the old raw[:4096]
+    # window while ET.fromstring still parsed and EXPANDED the entities —
+    # billion-laughs reopened. A legitimate sitemap contains neither construct
+    # anywhere, so a full case-insensitive scan is correct; over ≤20 MB it is a
+    # single re.search, microseconds, no 20 MB uppercased copy.
+    import re                                       # stdlib, lazy
+    if re.search(rb"(?i)<!\s*(DOCTYPE|ENTITY)", raw):
         raise UnsafeXML("DTD in sitemap")
 
 SITEMAP_NS = "{http://www.sitemaps.org/schemas/sitemap/0.9}"
diff --git a/lib/url-guard.sh b/lib/url-guard.sh
index 61a65cf..587c01b 100644
--- a/lib/url-guard.sh
+++ b/lib/url-guard.sh
@@ -16,11 +16,13 @@
 # vault and into a request. Allowlist, per CLAUDE.md: explicit allowlist beats
 # implicit denylist.
 #
-# NOT COVERED, deliberately: DNS-level SSRF. A public hostname that RESOLVES to
-# a private address passes this guard. Closing that needs resolve-then-pin at
-# the HTTP layer; curl in a shell cannot do it without a TOCTOU window between
-# the check and the connection. Literal local targets ARE rejected below. The
-# omission is stated rather than silent — see lib/seo-data/README.md.
+# DNS-level SSRF (a public hostname that RESOLVES to a private address, or
+# rebinds between check and connect): this NAME-level guard does not catch it —
+# closing it needs resolve-then-pin at the HTTP layer. That is now DONE for the
+# Python egress: lib/seo-data/safe_fetch.py pins every fetch (sitemap, linkgraph,
+# rendercheck, drift). It is NOT done for shell `curl`, which cannot pin without
+# `curl --resolve`; those paths keep this literal-local check only. Stated, not
+# silent — see lib/seo-data/README.md (safe_fetch).
 set -uo pipefail
 
 _die() { echo "url-guard: $1" >&2; exit 2; }