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,15 +239,27 @@ 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 ""
+	}
+	pix, err := io.ReadAll(io.LimitReader(res.Body, fetchedPicMaxBytes))
 	if err != nil {
 		return ""
 	}

pkg/htmltext/htmltext_test.go

@@ -21,8 +21,11 @@ package htmltext
 
 import (
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -331,3 +334,56 @@ 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")
+	}
+}
+
+func TestPicClientHasTimeout(t *testing.T) {
+	// Guards against regressing to a bare &http.Client{}, which has no timeout.
+	assert.NotZero(t, picClient.Timeout)
+}