Harden recipe fetcher: public-only check, cache copies, rate-limit slots

- IsPublic now refuses every non-globally-reachable IPv4 special-purpose range (adds 192.0.0.0/24, TEST-NET-1/2/3, 192.88.99.0/24, benchmarking 198.18.0.0/15) and admits IPv6 only from global unicast 2000::/3, minus its documentation, 6to4, Teredo and IETF protocol-assignment ranges. Test fixtures move off TEST-NET-3 to public addresses; Fetch-level tests cover the new blocked ranges and the public IPv6 path. - The cache stores and returns copies of Page.Body, so callers cannot alter cached bytes or race with other readers. - The per-host rate limiter advances only when a request is let through; a wait that ends with its context holds no slot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 12:01:12 -04:00
parent e7dbcf3d2c
commit 177b2dd2e7
3 changed files with 165 additions and 49 deletions
@@ -210,15 +210,19 @@ automatic** (D-021).
  (`searxng` or `brave`, per `recipe_search_settings`).
 - Fetching is **SSRF-safe** (`recipes.Fetcher`): https:// only, at every
  redirect hop. Its dialer resolves the host once and refuses unless every
-  answer is public — loopback, RFC 1918, link-local, carrier-grade NAT
-  (100.64/10), multicast, unspecified, reserved, IPv6 unique-local,
-  link-local and multicast, and IPv4-mapped or IPv4-embedding (NAT64, 6to4,
-  Teredo) forms are refused — then dials the address it checked, so a
+  answer is public — every non-globally-reachable IPv4 special-purpose
+  range (loopback, RFC 1918, link-local, carrier-grade NAT, documentation
+  TEST-NETs, benchmarking 198.18/15, multicast, reserved and more) is
+  refused, IPv4-mapped forms included, and IPv6 must be global unicast
+  (2000::/3) outside documentation, 6to4, Teredo and other special ranges —
+  then dials the address it checked, so a
  rebinding DNS answer cannot swap in a private one. Keep-alives and
  environment proxies are off, so every request dials and checks afresh. At
  most 3 redirects; a body over 2 MB is an error, never truncated; 10 s for
  the whole fetch; a descriptive `User-Agent`; requests to one host at least
-  1 s apart; 2xx pages cached by URL for an hour (64 pages, oldest evicted).
+  1 s apart, counted from requests actually sent (an abandoned wait holds no
+  slot); 2xx pages cached by URL for an hour (64 pages, oldest evicted), and
+  callers get their own copy of a cached body.
 - Recipe data is parsed from schema.org `Recipe` JSON-LD.
 - Photos are cached locally (`photos`) and processed with
  `golang.org/x/image`.
@@ -2,6 +2,7 @@
 package recipes

 import (
+	"bytes"
 	"context"
 	"crypto/tls"
 	"errors"
@@ -97,8 +98,10 @@ type Fetcher struct {
 	// sleep waits d or until ctx is done; tests replace it.
 	sleep func(ctx context.Context, d time.Duration) error

-	mu       sync.Mutex
-	nextSlot map[string]time.Time
+	mu sync.Mutex
+	// lastSent is when each host's latest request was let through; only
+	// requests actually sent advance it.
+	lastSent map[string]time.Time
 	cache    map[string]cacheEntry
 }

@@ -121,7 +124,7 @@ func newFetcher(resolver Resolver, dial dialFunc, tlsConfig *tls.Config) *Fetche
 		timeout:  Timeout,
 		now:      time.Now,
 		sleep:    sleep,
-		nextSlot: map[string]time.Time{},
+		lastSent: map[string]time.Time{},
 		cache:    map[string]cacheEntry{},
 	}
 	transport := &http.Transport{
@@ -140,7 +143,8 @@ func newFetcher(resolver Resolver, dial dialFunc, tlsConfig *tls.Config) *Fetche
 }

 // Fetch returns the page at rawURL, from the cache when it was fetched
-// within CacheTTL. Only 2xx responses are returned and cached.
+// within CacheTTL. Only 2xx responses are returned and cached. The
+// returned Body is the caller's own copy.
 func (f *Fetcher) Fetch(ctx context.Context, rawURL string) (Page, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -208,11 +212,15 @@ func (f *Fetcher) cached(key string) (Page, bool) {
 		delete(f.cache, key)
 		return Page{}, false
 	}
-	return e.page, true
+	page := e.page
+	page.Body = bytes.Clone(page.Body)
+	return page, true
 }

-// store caches page under key, evicting the oldest entry when full.
+// store caches a copy of page under key, evicting the oldest entry when
+// full.
 func (f *Fetcher) store(key string, page Page) {
+	page.Body = bytes.Clone(page.Body)
 	f.mu.Lock()
 	defer f.mu.Unlock()
 	if _, ok := f.cache[key]; !ok && len(f.cache) >= CacheSize {
@@ -228,21 +236,26 @@ func (f *Fetcher) store(key string, page Page) {
 	f.cache[key] = cacheEntry{page: page, fetched: f.now()}
 }

-// waitTurn blocks until host's next request slot, keeping requests to one
-// host at least HostInterval apart.
+// waitTurn blocks until a request to host may be sent, keeping requests to
+// one host at least HostInterval apart. A wait holds no reservation: the
+// host's slot advances only when a request is let through, so a wait that
+// ends with ctx does not delay later requests.
 func (f *Fetcher) waitTurn(ctx context.Context, host string) error {
-	f.mu.Lock()
-	now := f.now()
-	slot := f.nextSlot[host]
-	if slot.Before(now) {
-		slot = now
+	for {
+		f.mu.Lock()
+		now := f.now()
+		last, sent := f.lastSent[host]
+		wait := last.Add(HostInterval).Sub(now)
+		if !sent || wait <= 0 {
+			f.lastSent[host] = now
+			f.mu.Unlock()
+			return nil
+		}
+		f.mu.Unlock()
+		if err := f.sleep(ctx, wait); err != nil {
+			return err
+		}
 	}
-	f.nextSlot[host] = slot.Add(HostInterval)
-	f.mu.Unlock()
-	if wait := slot.Sub(now); wait > 0 {
-		return f.sleep(ctx, wait)
-	}
-	return nil
 }

 // dialPublic resolves address's host once, refuses unless every answer is
@@ -321,38 +334,58 @@ func checkRedirect(req *http.Request, via []*http.Request) error {
 	return checkURL(req.URL)
 }

-// blockedPrefixes are the non-public ranges. IPv4-mapped IPv6 addresses
-// are unmapped before the check, so the IPv4 entries cover them too.
-var blockedPrefixes = []netip.Prefix{
-	netip.MustParsePrefix("0.0.0.0/8"),       // unspecified, "this network"
+// blockedIPv4 are the IPv4 special-purpose ranges that are not globally
+// reachable (IANA IPv4 Special-Purpose Address Registry). IPv4-mapped IPv6
+// addresses are unmapped before the check, so these cover them too.
+var blockedIPv4 = []netip.Prefix{
+	netip.MustParsePrefix("0.0.0.0/8"),       // "this network"
 	netip.MustParsePrefix("10.0.0.0/8"),      // RFC 1918
 	netip.MustParsePrefix("100.64.0.0/10"),   // carrier-grade NAT
 	netip.MustParsePrefix("127.0.0.0/8"),     // loopback
 	netip.MustParsePrefix("169.254.0.0/16"),  // link-local
 	netip.MustParsePrefix("172.16.0.0/12"),   // RFC 1918
+	netip.MustParsePrefix("192.0.0.0/24"),    // IETF protocol assignments
+	netip.MustParsePrefix("192.0.2.0/24"),    // documentation, TEST-NET-1
+	netip.MustParsePrefix("192.88.99.0/24"),  // 6to4 relay anycast (deprecated)
 	netip.MustParsePrefix("192.168.0.0/16"),  // RFC 1918
+	netip.MustParsePrefix("198.18.0.0/15"),   // benchmarking
+	netip.MustParsePrefix("198.51.100.0/24"), // documentation, TEST-NET-2
+	netip.MustParsePrefix("203.0.113.0/24"),  // documentation, TEST-NET-3
 	netip.MustParsePrefix("224.0.0.0/4"),     // multicast
 	netip.MustParsePrefix("240.0.0.0/4"),     // reserved, broadcast
-	netip.MustParsePrefix("::/96"),           // unspecified, loopback, IPv4-compatible
-	netip.MustParsePrefix("fc00::/7"),        // unique local
-	netip.MustParsePrefix("fe80::/10"),       // link-local
-	netip.MustParsePrefix("fec0::/10"),       // site-local (deprecated)
-	netip.MustParsePrefix("ff00::/8"),        // multicast
-	netip.MustParsePrefix("64:ff9b::/96"),    // NAT64, embeds any IPv4
-	netip.MustParsePrefix("64:ff9b:1::/48"),  // local-use NAT64
-	netip.MustParsePrefix("2002::/16"),       // 6to4, embeds any IPv4
-	netip.MustParsePrefix("2001::/32"),       // Teredo, embeds any IPv4
-	netip.MustParsePrefix("::ffff:0:0:0/96"), // IPv4-translated
+}
+
+// globalIPv6 is the only IPv6 range dialed: global unicast. Everything
+// outside it is refused, including ::, ::1, IPv4-compatible and
+// IPv4-translated forms, NAT64 (64:ff9b::/96), discard-only (100::/64),
+// unique local (fc00::/7), link-local (fe80::/10) and multicast (ff00::/8).
+var globalIPv6 = netip.MustParsePrefix("2000::/3")
+
+// blockedIPv6 are the special-purpose ranges inside global unicast that are
+// not public or embed an arbitrary IPv4 address.
+var blockedIPv6 = []netip.Prefix{
+	netip.MustParsePrefix("2001::/23"),     // IETF protocol assignments, Teredo, benchmarking
+	netip.MustParsePrefix("2001:db8::/32"), // documentation
+	netip.MustParsePrefix("2002::/16"),     // 6to4, embeds any IPv4
+	netip.MustParsePrefix("3fff::/20"),     // documentation
 }

 // IsPublic reports whether ip is a public unicast address the Fetcher may
-// dial.
+// dial. IPv4 addresses are public unless in a special-purpose range; IPv6
+// addresses must be global unicast and outside its special-purpose ranges.
 func IsPublic(ip netip.Addr) bool {
 	if !ip.IsValid() {
 		return false
 	}
 	ip = ip.Unmap().WithZone("")
-	for _, p := range blockedPrefixes {
+	blocked := blockedIPv4
+	if ip.Is6() {
+		if !globalIPv6.Contains(ip) {
+			return false
+		}
+		blocked = blockedIPv6
+	}
+	for _, p := range blocked {
 		if p.Contains(ip) {
 			return false
 		}
@@ -60,8 +60,13 @@ type testEnv struct {
 	dialed []string
 }

-// publicIP is a documentation address the tests treat as a public host.
-const publicIP = "203.0.113.10"
+// publicIP, publicIP2 and publicIPv6 are public addresses; the test dialer
+// routes them to the test server and never reaches the real hosts.
+const (
+	publicIP   = "93.184.215.14"
+	publicIP2  = "93.184.215.20"
+	publicIPv6 = "2606:2800:21f::10"
+)

 func newTestEnv(t *testing.T, handler http.Handler, answers map[string][][]string) *testEnv {
 	t.Helper()
@@ -81,7 +86,7 @@ func newTestEnv(t *testing.T, handler http.Handler, answers map[string][][]strin
 		if err != nil {
 			return nil, err
 		}
-		if strings.HasPrefix(host, "203.0.113.") {
+		if host == publicIP || host == publicIP2 || host == publicIPv6 {
 			var d net.Dialer
 			return d.DialContext(ctx, network, server.Listener.Addr().String())
 		}
@@ -128,6 +133,19 @@ func TestFetchReturnsPage(t *testing.T) {
 	}
 }

+func TestFetchDialsPublicIPv6(t *testing.T) {
+	t.Parallel()
+	env := newTestEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "ok")
+	}), map[string][][]string{"example.com": {{publicIPv6}}})
+	if _, err := env.f.Fetch(t.Context(), "https://example.com/"); err != nil {
+		t.Fatalf("Fetch: %v", err)
+	}
+	if got := env.dials(); len(got) != 1 || got[0] != "["+publicIPv6+"]:443" {
+		t.Errorf("dialed %v, want [[%s]:443]", got, publicIPv6)
+	}
+}
+
 func TestFetchRejectsNonHTTPS(t *testing.T) {
 	t.Parallel()
 	env := newTestEnv(t, http.NotFoundHandler(), publicAnswers())
@@ -159,8 +177,15 @@ var blockedIPs = []string{
 	"224.0.0.1", "239.255.255.250", "ff02::1", "::ffff:224.0.0.251", // multicast
 	"0.0.0.0", "0.1.2.3", "::", "::ffff:0.0.0.0", // unspecified
 	"fc00::1", "fd12:3456:789a::1", // unique local
-	"255.255.255.255", "240.0.0.1", "fec0::1",
-	"64:ff9b::a00:1", "2002:a00:1::1", "2001:0:a00:1::1", "::a00:1",
+	"255.255.255.255", "240.0.0.1", "fec0::1", // reserved, site-local
+	"192.0.0.1", "192.0.0.255", // IETF protocol assignments
+	"192.0.2.1", "198.51.100.7", "203.0.113.10", "::ffff:203.0.113.10", // documentation
+	"192.88.99.1",                                       // 6to4 relay anycast
+	"198.18.0.1", "198.19.255.255", "::ffff:198.18.0.1", // benchmarking
+	"2001:db8::1", "3fff::1", // IPv6 documentation
+	"2001:2::1", "100::1", // IPv6 benchmarking, discard-only
+	"::ffff:0:a00:1", // IPv4-translated
+	"64:ff9b::a00:1", "64:ff9b:1::a00:1", "2002:a00:1::1", "2001:0:a00:1::1", "::a00:1",
 }

 func TestIsPublic(t *testing.T) {
@@ -170,7 +195,12 @@ func TestIsPublic(t *testing.T) {
 			t.Errorf("IsPublic(%s) = true, want false", s)
 		}
 	}
-	for _, s := range []string{"8.8.8.8", "203.0.113.10", "100.63.255.255", "100.128.0.1", "172.32.0.1", "2606:4700::1111", "::ffff:8.8.8.8"} {
+	for _, s := range []string{
+		"8.8.8.8", publicIP, "::ffff:8.8.8.8", "100.63.255.255", "100.128.0.1", "172.32.0.1",
+		"192.0.1.255", "192.0.3.1", "192.88.98.255", "198.17.255.255", "198.20.0.1",
+		"198.51.99.255", "203.0.112.255", "203.0.114.1", "223.255.255.254",
+		"2606:4700::1111", publicIPv6, "2001:4860:4860::8888", "2a00:1450::1",
+	} {
 		if !IsPublic(netip.MustParseAddr(s)) {
 			t.Errorf("IsPublic(%s) = false, want true", s)
 		}
@@ -269,7 +299,7 @@ func TestFetchFollowsAtMostThreeRedirects(t *testing.T) {
 	t.Parallel()
 	answers := map[string][][]string{
 		"example.com":         {{publicIP}},
-		"recipes.example.com": {{"203.0.113.20"}},
+		"recipes.example.com": {{publicIP2}},
 	}
 	env := newTestEnv(t, chainHandler, answers)
 	page, err := env.f.Fetch(t.Context(), "https://example.com/hop/3")
@@ -280,7 +310,7 @@ func TestFetchFollowsAtMostThreeRedirects(t *testing.T) {
 		t.Errorf("page = %+v", page)
 	}
 	// Every hop is dialed, so every hop's address is checked.
-	if got := env.dials(); len(got) != 4 || got[0] != publicIP+":443" || got[3] != "203.0.113.20:443" {
+	if got := env.dials(); len(got) != 4 || got[0] != publicIP+":443" || got[3] != publicIP2+":443" {
 		t.Errorf("dialed %v, want 4 checked hops", got)
 	}

@@ -494,7 +524,7 @@ func TestFetchRateLimitsPerHost(t *testing.T) {
 	t.Parallel()
 	answers := map[string][][]string{
 		"example.com":         {{publicIP}},
-		"recipes.example.com": {{"203.0.113.20"}},
+		"recipes.example.com": {{publicIP2}},
 	}
 	env := newTestEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		fmt.Fprint(w, "ok")
@@ -513,3 +543,52 @@ func TestFetchRateLimitsPerHost(t *testing.T) {
 		t.Errorf("waits = %v, want [%s %s]", got, HostInterval, HostInterval)
 	}
 }
+
+func TestFetchCacheIsNotSharedWithCallers(t *testing.T) {
+	t.Parallel()
+	env := newTestEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "tacos")
+	}), publicAnswers())
+
+	for range 2 {
+		page, err := env.f.Fetch(t.Context(), "https://example.com/tacos")
+		if err != nil {
+			t.Fatalf("Fetch: %v", err)
+		}
+		if string(page.Body) != "tacos" {
+			t.Fatalf("body = %q, want the cached page unchanged", page.Body)
+		}
+		// Overwrite the returned body: neither the fetched nor the cached
+		// copy may reach the cache.
+		copy(page.Body, "XXXXX")
+	}
+}
+
+// TestFetchCanceledWaitHoldsNoSlot covers a burst of fetches whose
+// rate-limit waits end with their context: they must not push back the
+// next request that is allowed through.
+func TestFetchCanceledWaitHoldsNoSlot(t *testing.T) {
+	t.Parallel()
+	env := newTestEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "ok")
+	}), publicAnswers())
+	clock := installClock(env.f)
+
+	if _, err := env.f.Fetch(t.Context(), "https://example.com/first"); err != nil {
+		t.Fatalf("first Fetch: %v", err)
+	}
+	env.f.sleep = func(context.Context, time.Duration) error { return context.DeadlineExceeded }
+	for i := range 5 {
+		if _, err := env.f.Fetch(t.Context(), fmt.Sprintf("https://example.com/burst/%d", i)); !errors.Is(err, context.DeadlineExceeded) {
+			t.Fatalf("burst Fetch %d error = %v, want context.DeadlineExceeded", i, err)
+		}
+	}
+	env.f.sleep = clock.sleep
+	clock.advance(HostInterval)
+	if _, err := env.f.Fetch(t.Context(), "https://example.com/after"); err != nil {
+		t.Fatalf("Fetch after the burst: %v", err)
+	}
+	if got := clock.waits(); len(got) != 0 {
+		t.Errorf("waits = %v, want none: canceled waits held slots", got)
+	}
+}