[SiriusTL666]

Description

htmltext.GetPicByUrl used http.Get, which goes through http.DefaultClient. The zero value of http.Client has no timeout, so a slow or blackholed host blocks the calling goroutine until the OS gives up.

This is reachable by anyone, not just an admin action. internal/router/ui.go serves /favicon.ico like this:

if branding != nil && branding.Favicon != "" {
    c.String(http.StatusOK, htmltext.GetPicByUrl(branding.Favicon))
    return
}

The branding URL is operator-configured, but the route is unauthenticated. If that URL points at an unresponsive host, every anonymous request to /favicon.ico parks a handler goroutine and a connection with no upper bound — a cheap way to exhaust the server from the outside without a valid account.

Changes

• Use a package-level picClient with a 15s timeout, matching the existing remoteVersion fetch in dashboard_service.go.
• Treat non-200 responses as empty rather than returning the error body as if it were image data.
• Cap the read at 5MB via io.LimitReader, so a misconfigured or hostile URL cannot exhaust memory.

Verification

Built and tested with Go 1.25.0:

go test ./pkg/htmltext/ 
ok  github.com/apache/answer/pkg/htmltext  1.420s
gofmt -l pkg/htmltext/   # (no output)

Four new tests: success body, non-200 handling, the timeout itself, and a guard asserting the client keeps a non-zero timeout.

The timeout test was verified in both directions — it is a real regression test, not just a green check:

Code	Result
http.Get(url) (before)	--- FAIL: TestGetPicByUrlTimesOut (10.00s) — "did not honour the client timeout"
picClient.Get(url) (after)	--- PASS: TestGetPicByUrlTimesOut (0.10s)

Notes

No existing issue covers this; I found it while auditing Apache projects for unbounded outbound requests. Happy to open an issue first if that is preferred.

A related observation I did not change, since it is a behaviour question rather than a bug: this route also means an anonymous visitor can make the server issue an outbound request to whatever host the favicon URL names (SSRF surface for internal addresses). Fetching and caching the icon at configuration time, instead of per request, would remove both the latency and the SSRF surface. Out of scope here — happy to follow up if maintainers want it.

[SiriusTL666]

A couple of notes that may make review easier.

Is the favicon URL attacker-controlled, or only operator-controlled?

Only the operator sets branding.Favicon (it comes from site settings), so this is not a case of an anonymous user supplying a URL. What makes it worth fixing is the combination of two facts:

1. GetPicByUrl used http.Get → http.DefaultClient, whose zero value has no timeout.
2. /favicon.ico calls it on every request, unauthenticated.

So a single unresponsive or hanging favicon host turns each anonymous /favicon.ico hit into a parked goroutine plus an open connection, with no upper bound. The trigger is cheap; the amplification is what makes it a availability problem rather than just a slow page.

Why I also capped the body at 5MB

io.ReadAll on an unbounded response from a remote host is the same class of issue from the memory side — a misconfigured URL pointing at something large would be buffered fully into memory. 5MB comfortably covers any favicon. If you would rather keep the change minimal and drop this part, I can split it out.

Why I did not fix the SSRF surface

An anonymous request can make the server issue a GET to whatever host the favicon URL names, including internal addresses. The clean fix is to fetch and cache the icon when the setting is saved, instead of per request — but that changes when the fetch happens (and what happens on failure at save time), so it is a behaviour decision rather than a bug fix. Flagging it rather than deciding unilaterally.

Verification recap

• go test ./pkg/htmltext/ → ok (1.42s); gofmt -l clean.
• The timeout test fails without the fix (FAIL ... 10.00s) and passes with it (0.10s), so it is a real regression test rather than a green check.
• New tests cover success, non-200, the timeout, and that the client keeps a non-zero timeout.

Happy to adjust scope, split the change, or add an issue first if that is the preferred workflow.