url previews: stop dropping thumbnails on body cap and HEAD-403 CDNs

Two independent causes, both silent: LimitedBody returned an error once the cap was hit, so scrapeOG failed the whole goquery parse on any page over 2 MiB — even though og: tags live in <head>, near the top. Hitting the cap is a truncation, not a failure: return io.EOF and let the parser decide whether it found what it needed. Sized against the pages actually posted here (n=14): og:title landed within the first 7 KiB on twelve, worst case ~600 KiB. validateImageURL HEAD-probed the image and bailed on non-200. Some publisher CDNs (dims.apnews.com among them) answer HEAD with 403 while serving the same URL over GET, so their thumbnails were always dropped. Probe with HEAD first, fall back to a ranged GET asking for the first KiB. A 206 declares the full size in Content-Range's "/119070" suffix rather than Content-Length, so the tracking-pixel filter reads size from there.
2026-07-15 00:32:40 +00:00 · 2026-07-09 18:52:32 -07:00
parent b5493a0e79
commit ba7b20dfe5
5 changed files with 201 additions and 26 deletions
--- a/internal/plugin/link_thumbnail.go
+++ b/internal/plugin/link_thumbnail.go
@@ -72,37 +72,93 @@ func normalizeImageURL(raw string) string {
 	return u.String()
 }

-// validateImageURL HEAD-probes an image URL: it must be http(s), return 200,
-// have an image/* content type, and (if a length is declared) exceed 5 KiB so
-// tracking pixels are filtered. Returns false with no error on any failure.
+// trackingPixelBytes is the size at or below which a declared image is assumed
+// to be a tracking pixel rather than a real preview thumbnail.
+const trackingPixelBytes = 5120
+
+// declaredSize reports the full size of the resource a probe response describes,
+// or -1 when the origin declares none. A ranged probe answers 206 with a
+// Content-Length covering only the requested slice, so the total comes from
+// Content-Range's "bytes 0-1023/119070" suffix instead.
+func declaredSize(resp *http.Response) int64 {
+	if resp.StatusCode == http.StatusPartialContent {
+		cr := resp.Header.Get("Content-Range")
+		i := strings.LastIndexByte(cr, '/')
+		if i < 0 {
+			return -1
+		}
+		total, err := strconv.ParseInt(strings.TrimSpace(cr[i+1:]), 10, 64)
+		if err != nil {
+			return -1 // "*" or malformed: unknown, not empty
+		}
+		return total
+	}
+	if cl := resp.Header.Get("Content-Length"); cl != "" {
+		if size, err := strconv.ParseInt(cl, 10, 64); err == nil {
+			return size
+		}
+	}
+	return -1
+}
+
+// probeImage asks the origin about rawURL using method and reports its content
+// type and full size. GET probes request only the first KiB, so falling back to
+// one stays about as cheap as the HEAD it replaces.
+func probeImage(method, rawURL string) (contentType string, size int64, ok bool) {
+	req, err := http.NewRequest(method, rawURL, nil)
+	if err != nil {
+		return "", -1, false
+	}
+	req.Header.Set("User-Agent", thumbnailUserAgent)
+	if method == http.MethodGet {
+		req.Header.Set("Range", "bytes=0-1023")
+	}
+
+	resp, err := thumbnailClient.Do(req)
+	if err != nil {
+		slog.Debug("thumbnail: image probe failed", "method", method, "url", rawURL, "err", err)
+		return "", -1, false
+	}
+	defer resp.Body.Close()
+	// Drain the sipped bytes so the connection can be reused.
+	_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
+
+	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusPartialContent {
+		slog.Debug("thumbnail: image probe rejected", "method", method, "url", rawURL, "status", resp.StatusCode)
+		return "", -1, false
+	}
+	return resp.Header.Get("Content-Type"), declaredSize(resp), true
+}
+
+// validateImageURL probes an image URL: it must be http(s), answer a probe, have
+// an image/* content type, and (if a size is declared) exceed trackingPixelBytes.
+// Returns false with no error on any failure.
+//
+// The probe is a HEAD first, falling back to a ranged GET: CDNs in front of some
+// publishers (apnews's dims.apnews.com among them) answer HEAD with 403 while
+// serving the very same URL over GET, and a HEAD-only check silently drops those
+// thumbnails.
 func validateImageURL(rawURL string) bool {
 	if rawURL == "" || safehttp.ValidateURL(rawURL) != nil {
 		return false
 	}
-	req, err := http.NewRequest("HEAD", rawURL, nil)
-	if err != nil {
-		return false
-	}
-	req.Header.Set("User-Agent", thumbnailUserAgent)

-	resp, err := thumbnailClient.Do(req)
-	if err != nil {
-		slog.Debug("thumbnail: image HEAD failed", "url", rawURL, "err", err)
+	contentType, size, ok := probeImage(http.MethodHead, rawURL)
+	if !ok {
+		contentType, size, ok = probeImage(http.MethodGet, rawURL)
+	}
+	if !ok {
 		return false
 	}
-	defer resp.Body.Close()

-	if resp.StatusCode != http.StatusOK {
+	if !strings.HasPrefix(contentType, "image/") {
+		slog.Debug("thumbnail: not an image", "url", rawURL, "content_type", contentType)
 		return false
 	}
-	if !strings.HasPrefix(resp.Header.Get("Content-Type"), "image/") {
+	if size >= 0 && size <= trackingPixelBytes {
+		slog.Debug("thumbnail: image too small, treating as tracking pixel", "url", rawURL, "size", size)
 		return false
 	}
-	if cl := resp.Header.Get("Content-Length"); cl != "" {
-		if size, err := strconv.ParseInt(cl, 10, 64); err == nil && size <= 5120 {
-			return false // tracking pixel
-		}
-	}
 	return true
 }

--- a/internal/plugin/link_thumbnail_test.go
+++ b/internal/plugin/link_thumbnail_test.go
@@ -1,6 +1,9 @@
 package plugin

-import "testing"
+import (
+	"net/http"
+	"testing"
+)

 func TestResolveURL(t *testing.T) {
 	cases := []struct {
@@ -38,3 +41,51 @@ func TestNormalizeImageURL(t *testing.T) {
 		t.Errorf("normalizeImageURL unsigned = %q, want width=1200", got)
 	}
 }
+
+func TestDeclaredSize(t *testing.T) {
+	cases := []struct {
+		name   string
+		status int
+		header map[string]string
+		want   int64
+	}{
+		{"content-length on a full response", http.StatusOK,
+			map[string]string{"Content-Length": "119070"}, 119070},
+		// A ranged probe's Content-Length covers only the slice we asked for, so
+		// the resource's real size has to come from Content-Range's total.
+		{"content-range total on a partial response", http.StatusPartialContent,
+			map[string]string{"Content-Length": "1024", "Content-Range": "bytes 0-1023/119070"}, 119070},
+		{"unknown total in content-range", http.StatusPartialContent,
+			map[string]string{"Content-Range": "bytes 0-1023/*"}, -1},
+		{"partial response missing content-range", http.StatusPartialContent,
+			map[string]string{"Content-Length": "1024"}, -1},
+		{"no size declared", http.StatusOK, map[string]string{}, -1},
+		{"malformed content-length", http.StatusOK,
+			map[string]string{"Content-Length": "banana"}, -1},
+	}
+	for _, c := range cases {
+		resp := &http.Response{StatusCode: c.status, Header: http.Header{}}
+		for k, v := range c.header {
+			resp.Header.Set(k, v)
+		}
+		if got := declaredSize(resp); got != c.want {
+			t.Errorf("%s: declaredSize() = %d, want %d", c.name, got, c.want)
+		}
+	}
+}
+
+// An undeclared size (-1) must not be mistaken for a zero-byte image and dropped
+// as a tracking pixel — plenty of CDNs omit the length entirely.
+func TestTrackingPixelFilterSkipsUnknownSize(t *testing.T) {
+	rejected := func(size int64) bool { return size >= 0 && size <= trackingPixelBytes }
+	for _, size := range []int64{-1, trackingPixelBytes + 1, 119070} {
+		if rejected(size) {
+			t.Errorf("size %d was rejected as a tracking pixel, want kept", size)
+		}
+	}
+	for _, size := range []int64{0, 1, trackingPixelBytes} {
+		if !rejected(size) {
+			t.Errorf("size %d was kept, want rejected as a tracking pixel", size)
+		}
+	}
+}
--- a/internal/plugin/urls.go
+++ b/internal/plugin/urls.go
@@ -187,7 +187,14 @@ func (p *URLsPlugin) scrapeOG(rawURL string) (string, string, string, error) {
 		return "", "", "", fmt.Errorf("status %d", resp.StatusCode)
 	}

-	// Cap the parsed body at 2 MiB — og: tags live in <head>, near the top.
+	// Cap the parsed body at 2 MiB: big enough that <head> always fits, small
+	// enough to bound memory against an origin streaming an endless body. Reaching
+	// the cap truncates rather than failing, so an oversized tail costs nothing.
+	//
+	// Sized against the pages actually posted here (2026-07-09, n=14): og:title
+	// landed within the first 7 KiB on twelve of them, worst case ~600 KiB, on
+	// pages up to 1.44 MiB. Note failed scrapes never reach url_cache, so that
+	// sample can't tell you how big the pages that *blew* the old cap were.
 	doc, err := goquery.NewDocumentFromReader(safehttp.LimitedBody(resp.Body, 2*1024*1024))
 	if err != nil {
 		return "", "", "", fmt.Errorf("parse HTML: %w", err)
--- a/internal/safehttp/safehttp.go
+++ b/internal/safehttp/safehttp.go
@@ -120,10 +120,16 @@ func NewClient(timeout time.Duration) *http.Client {
 	}
 }

-// LimitedBody wraps r in a reader that errors once more than max bytes have
-// been read. Use to cap how much of a response body downstream parsers
-// (goquery, image.Decode) will ever see — a hostile origin streaming an
-// endless body otherwise OOMs the process.
+// LimitedBody wraps r in a reader that reports EOF once max bytes have been
+// read. Use to cap how much of a response body downstream parsers (goquery,
+// image.Decode) will ever see — a hostile origin streaming an endless body
+// otherwise OOMs the process.
+//
+// Hitting the cap is a truncation, not an error: parsers get a short-but-valid
+// body and decide for themselves whether they found what they needed. Returning
+// an error here instead would fail the whole parse on any oversized page, even
+// when the interesting bytes (an HTML <head>, an image header) sit well inside
+// the cap.
 func LimitedBody(r io.Reader, max int64) io.Reader {
 	return &limitedReader{R: r, N: max}
 }
@@ -135,7 +141,7 @@ type limitedReader struct {

 func (l *limitedReader) Read(p []byte) (int, error) {
 	if l.N <= 0 {
-		return 0, fmt.Errorf("safehttp: response body exceeded cap")
+		return 0, io.EOF
 	}
 	if int64(len(p)) > l.N {
 		p = p[:l.N]
--- a/internal/safehttp/safehttp_test.go
+++ b/internal/safehttp/safehttp_test.go
@@ -0,0 +1,55 @@
+package safehttp
+
+import (
+	"io"
+	"strings"
+	"testing"
+)
+
+// A body under the cap is passed through untouched.
+func TestLimitedBodyUnderCap(t *testing.T) {
+	got, err := io.ReadAll(LimitedBody(strings.NewReader("hello"), 1024))
+	if err != nil {
+		t.Fatalf("ReadAll: %v", err)
+	}
+	if string(got) != "hello" {
+		t.Fatalf("got %q, want %q", got, "hello")
+	}
+}
+
+// A body over the cap truncates cleanly at EOF rather than failing the read.
+// Callers parse whatever fits (an HTML <head>, an image header) instead of
+// losing the whole document to an oversized tail.
+func TestLimitedBodyTruncatesAtEOF(t *testing.T) {
+	body := strings.Repeat("x", 5000)
+	got, err := io.ReadAll(LimitedBody(strings.NewReader(body), 100))
+	if err != nil {
+		t.Fatalf("ReadAll returned an error instead of truncating: %v", err)
+	}
+	if len(got) != 100 {
+		t.Fatalf("read %d bytes, want the 100-byte cap", len(got))
+	}
+}
+
+// The cap bounds total bytes across many small reads, not just a single one.
+func TestLimitedBodyCapsAcrossReads(t *testing.T) {
+	r := LimitedBody(strings.NewReader(strings.Repeat("x", 5000)), 10)
+	total := 0
+	buf := make([]byte, 3)
+	for {
+		n, err := r.Read(buf)
+		total += n
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			t.Fatalf("Read: %v", err)
+		}
+		if total > 10 {
+			t.Fatalf("read %d bytes past the 10-byte cap", total)
+		}
+	}
+	if total != 10 {
+		t.Fatalf("read %d bytes, want 10", total)
+	}
+}