diff --git a/internal/plugin/link_thumbnail.go b/internal/plugin/link_thumbnail.go index e247e46..4f46b18 100644 --- 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 } diff --git a/internal/plugin/link_thumbnail_test.go b/internal/plugin/link_thumbnail_test.go index 094e343..55e69b1 100644 --- 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) + } + } +} diff --git a/internal/plugin/urls.go b/internal/plugin/urls.go index 89772ad..5cbe1bf 100644 --- 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 , near the top. + // Cap the parsed body at 2 MiB: big enough that 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) diff --git a/internal/safehttp/safehttp.go b/internal/safehttp/safehttp.go index 0f8ccfe..2824495 100644 --- 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 , 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] diff --git a/internal/safehttp/safehttp_test.go b/internal/safehttp/safehttp_test.go new file mode 100644 index 0000000..1035e17 --- /dev/null +++ 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 , 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) + } +}