fix(branding): bound the favicon fetch timeout

pkg/htmltext/htmltext.go

@@ -26,6 +26,7 @@ import (
 	"regexp"
 	"strings"
 	"sync/atomic"
+	"time"
 	"unicode"
 	"unicode/utf8"
 
@@ -238,17 +239,34 @@ func FetchMatchedExcerpt(html string, words []string, trimMarker string, trimLen
 	return FetchRangedExcerpt(html, trimMarker, runeOffset, runeLimit)
 }
 
+// fetchedPicMaxBytes caps how much of a remote image is read, so a hostile or
+// misconfigured URL cannot exhaust memory.
+const fetchedPicMaxBytes = 5 << 20
+
+// picClient is used for anonymous, user-influenced outbound fetches. The zero
+// value of http.Client has no timeout, so a slow or blackholed host would block
+// the handler goroutine until the OS gives up.
+var picClient = &http.Client{Timeout: 15 * time.Second}
+
 func GetPicByUrl(url string) string {
-	res, err := http.Get(url)
+	res, err := picClient.Get(url)
 	if err != nil {
 		return ""
 	}
 	defer func() {
 		_ = res.Body.Close()
 	}()
-	pix, err := io.ReadAll(res.Body)
+	if res.StatusCode != http.StatusOK {
+		return ""
+	}
+	// Read one byte past the cap: LimitReader alone would silently truncate an
+	// oversized body and hand back a corrupt image as if the fetch succeeded.
+	pix, err := io.ReadAll(io.LimitReader(res.Body, fetchedPicMaxBytes+1))
 	if err != nil {
 		return ""
 	}
+	if len(pix) > fetchedPicMaxBytes {
+		return ""
+	}
 	return string(pix)
 }

pkg/htmltext/htmltext_test.go

@@ -20,9 +20,13 @@
 package htmltext
 
 import (
+	"bytes"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -331,3 +335,82 @@ func TestFetchMatchedExcerpt(t *testing.T) {
 	actual = FetchMatchedExcerpt(html, []string{"中文", "😂"}, "...", 6)
 	assert.Equal(t, expected, actual)
 }
+
+func TestGetPicByUrlReturnsBody(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte("image-bytes"))
+	}))
+	defer srv.Close()
+
+	assert.Equal(t, "image-bytes", GetPicByUrl(srv.URL))
+}
+
+func TestGetPicByUrlReturnsEmptyOnErrorStatus(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte("not found"))
+	}))
+	defer srv.Close()
+
+	assert.Equal(t, "", GetPicByUrl(srv.URL))
+}
+
+// A slow origin must not pin the handler goroutine indefinitely: /favicon.ico
+// calls GetPicByUrl on every anonymous request, so an unresponsive branding URL
+// would otherwise accumulate stuck goroutines with no upper bound.
+func TestGetPicByUrlTimesOut(t *testing.T) {
+	release := make(chan struct{})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-release
+	}))
+	defer func() {
+		close(release)
+		srv.Close()
+	}()
+
+	original := picClient.Timeout
+	picClient.Timeout = 100 * time.Millisecond
+	defer func() { picClient.Timeout = original }()
+
+	done := make(chan string, 1)
+	go func() { done <- GetPicByUrl(srv.URL) }()
+
+	select {
+	case got := <-done:
+		assert.Equal(t, "", got, "a timed-out fetch must yield an empty result")
+	case <-time.After(10 * time.Second):
+		t.Fatal("GetPicByUrl did not honour the client timeout")
+	}
+}
+
+// A body at the cap is still a complete image and must be served.
+func TestGetPicByUrlAcceptsBodyAtSizeLimit(t *testing.T) {
+	body := bytes.Repeat([]byte("a"), fetchedPicMaxBytes)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write(body)
+	}))
+	defer srv.Close()
+
+	got := GetPicByUrl(srv.URL)
+	assert.Len(t, got, fetchedPicMaxBytes)
+}
+
+// One byte past the cap means the response was truncated, so the bytes are not
+// a usable image. Handing them back as a success would serve a corrupt favicon.
+func TestGetPicByUrlRejectsBodyOverSizeLimit(t *testing.T) {
+	body := bytes.Repeat([]byte("a"), fetchedPicMaxBytes+1)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write(body)
+	}))
+	defer srv.Close()
+
+	assert.Equal(t, "", GetPicByUrl(srv.URL))
+}
+
+func TestPicClientHasTimeout(t *testing.T) {
+	// Guards against regressing to a bare &http.Client{}, which has no timeout.
+	assert.NotZero(t, picClient.Timeout)
+}