From ab61b8589ac7b897d6cfc036267321ed52cd809b Mon Sep 17 00:00:00 2001
From: SiriusTL666 <214846386+SiriusTL666@users.noreply.github.com>
Date: Sun, 25 Oct 2026 21:26:07 +0800
Subject: [PATCH] fix(branding): bound the favicon fetch timeout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

GetPicByUrl used http.Get, which goes through http.DefaultClient. The zero
value of http.Client has no timeout, so a branding URL pointing at a host that
is slow or blackholed blocks the handler goroutine until the OS gives up.

/favicon.ico calls this for every anonymous request, so an unreachable
favicon or square icon URL accumulates stuck goroutines and connections with
no upper bound — a cheap way to exhaust the server from the outside.

- Use a package-level client with a 15s timeout, matching the existing
  remoteVersion fetch in dashboard_service.
- Treat non-200 responses as empty instead of returning the error body as if it
  were image data.
- Cap the read at 5MB so a misconfigured or hostile URL cannot exhaust memory.
- Add tests covering success, error status, the timeout, and that the client
  keeps a non-zero timeout.
---
 pkg/htmltext/htmltext.go      | 17 +++++++++--
 pkg/htmltext/htmltext_test.go | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/pkg/htmltext/htmltext.go b/pkg/htmltext/htmltext.go
index 929080838..47daeef40 100644
--- a/pkg/htmltext/htmltext.go
+++ b/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 ""
 	}
diff --git a/pkg/htmltext/htmltext_test.go b/pkg/htmltext/htmltext_test.go
index bcedcb3c8..781d5ff12 100644
--- a/pkg/htmltext/htmltext_test.go
+++ b/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)
+}