diff --git a/pkg/htmltext/htmltext.go b/pkg/htmltext/htmltext.go index 929080838..a6e9d64c7 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,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) } diff --git a/pkg/htmltext/htmltext_test.go b/pkg/htmltext/htmltext_test.go index bcedcb3c8..da32956e2 100644 --- a/pkg/htmltext/htmltext_test.go +++ b/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) +}