fix(artwork): block private and loopback addresses in remote image fetches (#6181)

* fix(artwork): block private and loopback addresses in remote image fetches

fromURL fetched any URL with a plain HTTP client, and two untrusted inputs reach it. A playlist
can set #EXTALBUMARTURL to an http(s) URL, which the artwork worker later fetches when
EnableM3UExternalAlbumArt is on, so any user who can import a playlist controls the target.
Metadata agents, including WASM plugins without the http permission, return image URLs that the
core fetches too. Either path could make the server request loopback, LAN or link-local
addresses and store the response as artwork that is served back.

Add httpclient.NewExternal, which dials through a net.Dialer Control hook that rejects private,
loopback, link-local and unspecified addresses. The check runs at dial time on the resolved IP,
so DNS names, redirects and DNS rebinding are covered. fromURL now uses one shared client built
with it and treats a refused address as a definitive miss, so the item settles absent instead of
retrying and tripping the agent's circuit breaker. httpclient.New is unchanged for the other callers.

The IP classification moves from plugins to the new utils/netguard package, shared by the plugin
host client and the new constructor. The artwork test suite swaps in a client that allows
loopback so existing specs can keep using httptest servers; the fromURL specs use the production
client to assert the refusal.

* fix(httpclient): keep dialing a configured proxy in the guarded client

The guard runs on the resolved address, and with HTTP_PROXY set that address is the proxy, not
the image host. A proxy on a private address would have had every remote artwork fetch refused,
and a refusal settles the item as absent, so covers would silently disappear for those setups.

Dial the configured proxy endpoint directly and keep the guard for every other dial. A proxy
relays the request itself, so it is the operator's egress policy, the same one every other
httpclient.New caller already goes through.

* fix(httpclient): exempt only the hop that actually goes through the proxy

The exemption matched any dial to a configured proxy's address, but net/http never proxies
loopback targets, so a URL aimed at a loopback proxy was dialed directly and skipped the guard.
That let an image URL reach that one address.

Tag each request with the proxy it resolves to and exempt a dial only when it is that hop.
Redirects re-enter the RoundTripper, so every hop is tagged on its own.

core/artwork/artwork_suite_test.go

@@ -2,18 +2,21 @@ package artwork
 
 import (
 	"io/fs"
+	"net/netip"
 	"net/url"
 	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/navidrome/navidrome/core/storage"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/model/metadata"
 	"github.com/navidrome/navidrome/tests"
+	"github.com/navidrome/navidrome/utils/httpclient"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"go.uber.org/goleak"
@@ -37,6 +40,14 @@ func TestArtwork(t *testing.T) {
 	RunSpecs(t, "Artwork Suite")
 }
 
+// productionImageClient keeps the guarded client for the specs that assert it refuses loopback.
+var productionImageClient = remoteImageClient
+
+// httptest servers listen on loopback, which the production client refuses.
+var _ = BeforeSuite(func() {
+	remoteImageClient = httpclient.NewExternal(5*time.Second, netip.MustParsePrefix("127.0.0.0/8"), netip.MustParsePrefix("::1/128"))
+})
+
 // osDirFS wraps os.DirFS as a storage.MusicFS for integration tests.
 type osDirFS struct{ fs.FS }
 

core/artwork/sources.go

@@ -18,6 +18,7 @@ import (
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/utils/httpclient"
+	"github.com/navidrome/navidrome/utils/netguard"
 	"go.senan.xyz/taglib"
 )
 
@@ -150,10 +151,18 @@ type readCloser struct {
 	io.Closer
 }
 
+// remoteImageClient fetches URLs from playlists and agents (plugins included), so it must not reach
+// internal hosts. Shared so fetches reuse connections.
+var remoteImageClient = httpclient.NewExternal(5 * time.Second)
+
 func fromURL(ctx context.Context, imageUrl *url.URL) (io.ReadCloser, string, error) {
-	hc := httpclient.New(5 * time.Second)
 	req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageUrl.String(), nil)
-	resp, err := hc.Do(req) //nolint:gosec
+	resp, err := remoteImageClient.Do(req)
+	if errors.Is(err, netguard.ErrPrivateAddress) {
+		// Retrying cannot change where the URL points: settle absent instead of tripping the breaker.
+		log.Warn(ctx, "Artwork: Refused to fetch image from a private or loopback address", "url", imageUrl, err)
+		return nil, "", model.ErrNotFound
+	}
 	if err != nil {
 		return nil, "", err
 	}

core/artwork/sources_internal_test.go

@@ -5,13 +5,87 @@ import (
 	"errors"
 	"io"
 	"io/fs"
+	"net/http"
+	"net/http/httptest"
+	"net/netip"
+	"net/url"
 	"os"
+	"strings"
+	"sync/atomic"
 	"testing/fstest"
+	"time"
 
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils/httpclient"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
+var _ = Describe("fromURL", func() {
+	var (
+		hits   atomic.Int32
+		target *httptest.Server
+	)
+
+	BeforeEach(func() {
+		hits.Store(0)
+		target = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			hits.Add(1)
+			_, _ = w.Write([]byte("image-bytes"))
+		}))
+		DeferCleanup(target.Close)
+	})
+
+	useClient := func(c *http.Client) {
+		prev := remoteImageClient
+		remoteImageClient = c
+		DeferCleanup(func() { remoteImageClient = prev })
+	}
+	fetch := func(rawURL string) ([]byte, error) {
+		u, err := url.Parse(rawURL)
+		Expect(err).ToNot(HaveOccurred())
+		r, _, err := fromURL(GinkgoT().Context(), u)
+		if err != nil {
+			return nil, err
+		}
+		defer r.Close()
+		return io.ReadAll(r)
+	}
+	// Stand-in for a public host: only 127.0.0.1 is allowed, so every other private address stays refused.
+	onlyLocalhostV4 := func() *http.Client {
+		return httpclient.NewExternal(5*time.Second, netip.MustParsePrefix("127.0.0.1/32"))
+	}
+
+	DescribeTable("refuses private and loopback targets as a definitive miss",
+		func(rawURL string) {
+			useClient(productionImageClient)
+			u, _ := url.Parse(target.URL)
+			_, err := fetch(strings.ReplaceAll(rawURL, "PORT", u.Port()))
+			Expect(err).To(MatchError(model.ErrNotFound))
+			Expect(hits.Load()).To(BeZero())
+		},
+		Entry("IPv4 loopback", "http://127.0.0.1:PORT/x"),
+		Entry("localhost", "http://localhost:PORT/x"),
+		Entry("cloud metadata", "http://169.254.169.254/"),
+		Entry("IPv6 loopback", "http://[::1]/"),
+	)
+
+	It("refuses a redirect from an allowed host to a loopback address", func() {
+		useClient(onlyLocalhostV4())
+		redirector := httptest.NewServer(http.RedirectHandler(strings.Replace(target.URL, "127.0.0.1", "127.0.0.2", 1), http.StatusFound))
+		DeferCleanup(redirector.Close)
+
+		_, err := fetch(redirector.URL)
+		Expect(err).To(MatchError(model.ErrNotFound))
+		Expect(hits.Load()).To(BeZero())
+	})
+
+	It("fetches from an allowed address", func() {
+		useClient(onlyLocalhostV4())
+		Expect(fetch(target.URL + "/cover.jpg")).To(Equal([]byte("image-bytes")))
+	})
+})
+
 var _ = Describe("fromExternalFile", func() {
 	It("opens a matching file via the library FS", func() {
 		fsys := fstest.MapFS{

plugins/host_httpclient.go

@@ -16,6 +16,7 @@ import (
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/plugins/host"
 	"github.com/navidrome/navidrome/utils/httpclient"
+	"github.com/navidrome/navidrome/utils/netguard"
 )
 
 const (
@@ -201,7 +202,7 @@ func isPrivateOrLoopback(hostname string) bool {
 	if ip == nil {
 		return false
 	}
-	return isPrivateIP(ip)
+	return netguard.IsPrivateIP(ip)
 }
 
 // Verify interface implementation

plugins/host_netguard.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"net"
 	"slices"
+
+	"github.com/navidrome/navidrome/utils/netguard"
 )
 
 // checkPrivateDial runs at dial time on the resolved IP, so hostnames can't reach private addresses unless a
@@ -17,7 +19,7 @@ func checkPrivateDial(requiredHosts []string, address string) error {
 		return err
 	}
 	ip := net.ParseIP(host)
-	if ip == nil || !isPrivateIP(ip) {
+	if ip == nil || !netguard.IsPrivateIP(ip) {
 		return nil
 	}
 	for _, entry := range requiredHosts {
@@ -51,7 +53,3 @@ func ipMatchesEntry(entry string, ip net.IP) bool {
 	}
 	return false
 }
-
-func isPrivateIP(ip net.IP) bool {
-	return ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast()
-}

utils/httpclient/httpclient.go

@@ -3,10 +3,15 @@
 package httpclient
 
 import (
+	"context"
+	"net"
 	"net/http"
+	"net/netip"
+	"net/url"
 	"time"
 
 	"github.com/navidrome/navidrome/consts"
+	"github.com/navidrome/navidrome/utils/netguard"
 )
 
 type uaTransport struct {
@@ -33,3 +38,47 @@ func NewTransport(base http.RoundTripper) http.RoundTripper {
 func New(timeout time.Duration) *http.Client {
 	return &http.Client{Timeout: timeout, Transport: NewTransport(nil)}
 }
+
+// proxyFunc resolves the proxy for a request; tests replace it.
+var proxyFunc = http.ProxyFromEnvironment
+
+type proxyAddrKey struct{}
+
+// NewExternal is New for URLs from untrusted sources: it refuses to dial private, loopback,
+// link-local and unspecified addresses, except those covered by allowed.
+func NewExternal(timeout time.Duration, allowed ...netip.Prefix) *http.Client {
+	t := http.DefaultTransport.(*http.Transport).Clone()
+	t.Proxy = proxyFunc
+	direct := net.Dialer{Timeout: 30 * time.Second, KeepAlive: 30 * time.Second}
+	guarded := direct
+	guarded.Control = netguard.DialControl(allowed...)
+	t.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+		// Exempt the hop to the proxy, which relays the request and is operator config. A URL
+		// aimed at the proxy's own address is not proxied, so it stays guarded.
+		if proxy, ok := ctx.Value(proxyAddrKey{}).(string); ok && proxy == addr {
+			return direct.DialContext(ctx, network, addr)
+		}
+		return guarded.DialContext(ctx, network, addr)
+	}
+	return &http.Client{Timeout: timeout, Transport: NewTransport(&proxyTagger{base: t})}
+}
+
+// proxyTagger records the proxy each request resolves to, so the dialer can tell a hop to the
+// proxy from a dial to the URL's own host.
+type proxyTagger struct{ base http.RoundTripper }
+
+func (p *proxyTagger) RoundTrip(req *http.Request) (*http.Response, error) {
+	if u, err := proxyFunc(req); err == nil && u != nil {
+		req = req.WithContext(context.WithValue(req.Context(), proxyAddrKey{}, proxyAddr(u)))
+	}
+	return p.base.RoundTrip(req)
+}
+
+// proxyAddr mirrors how net/http addresses a proxy connection.
+func proxyAddr(u *url.URL) string {
+	port := u.Port()
+	if port == "" {
+		port = map[string]string{"http": "80", "https": "443", "socks5": "1080", "socks5h": "1080"}[u.Scheme]
+	}
+	return net.JoinHostPort(u.Hostname(), port)
+}

utils/httpclient/httpclient_proxy_internal_test.go

@@ -0,0 +1,68 @@
+package httpclient
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"sync/atomic"
+	"time"
+
+	"github.com/navidrome/navidrome/utils/netguard"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("NewExternal with a proxy", func() {
+	var proxy *httptest.Server
+	var proxied atomic.Int32
+	var proxyURL *url.URL
+
+	BeforeEach(func() {
+		proxied.Store(0)
+		proxy = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			proxied.Add(1)
+			_, _ = w.Write([]byte("via proxy"))
+		}))
+		DeferCleanup(proxy.Close)
+		proxyURL, _ = url.Parse(proxy.URL)
+		prev := proxyFunc
+		DeferCleanup(func() { proxyFunc = prev })
+	})
+
+	It("dials the configured proxy even though it listens on a private address", func() {
+		proxyFunc = func(*http.Request) (*url.URL, error) { return proxyURL, nil }
+
+		resp, err := NewExternal(time.Second).Get("http://navidrome.example.com/cover.jpg")
+		Expect(err).ToNot(HaveOccurred())
+		defer resp.Body.Close()
+		Expect(proxied.Load()).To(Equal(int32(1)))
+	})
+
+	// net/http never proxies loopback targets, so a URL aimed at a loopback proxy is dialed directly.
+	It("refuses a direct dial to the proxy's own address when the request is not proxied", func() {
+		proxyFunc = func(r *http.Request) (*url.URL, error) {
+			if r.URL.Hostname() == "127.0.0.1" {
+				return nil, nil
+			}
+			return proxyURL, nil
+		}
+
+		_, err := NewExternal(time.Second).Get(proxy.URL + "/secret")
+		Expect(err).To(MatchError(netguard.ErrPrivateAddress))
+		Expect(proxied.Load()).To(BeZero())
+	})
+
+	It("still refuses a direct dial to a private address", func() {
+		proxyFunc = func(r *http.Request) (*url.URL, error) {
+			if r.URL.Scheme == "https" {
+				return proxyURL, nil
+			}
+			return nil, nil
+		}
+		target := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
+		DeferCleanup(target.Close)
+
+		_, err := NewExternal(time.Second).Get(target.URL)
+		Expect(err).To(MatchError(netguard.ErrPrivateAddress))
+	})
+})

utils/httpclient/httpclient_test.go

@@ -3,10 +3,12 @@ package httpclient_test
 import (
 	"net/http"
 	"net/http/httptest"
+	"net/netip"
 	"time"
 
 	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/utils/httpclient"
+	"github.com/navidrome/navidrome/utils/netguard"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
@@ -16,6 +18,7 @@ var _ = Describe("httpclient", func() {
 	var receivedUA string
 
 	BeforeEach(func() {
+		receivedUA = ""
 		server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			receivedUA = r.Header.Get("User-Agent")
 		}))
@@ -48,6 +51,34 @@ var _ = Describe("httpclient", func() {
 		})
 	})
 
+	Describe("NewExternal", func() {
+		It("refuses to connect to a loopback server", func() {
+			c := httpclient.NewExternal(time.Second)
+			_, err := c.Get(server.URL)
+			Expect(err).To(MatchError(netguard.ErrPrivateAddress))
+			Expect(receivedUA).To(BeEmpty())
+		})
+
+		It("refuses a redirect from an allowed host to a private address", func() {
+			redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound)
+			}))
+			DeferCleanup(redirector.Close)
+
+			c := httpclient.NewExternal(time.Second, netip.MustParsePrefix("127.0.0.1/32"))
+			_, err := c.Get(redirector.URL)
+			Expect(err).To(MatchError(netguard.ErrPrivateAddress))
+		})
+
+		It("connects to addresses covered by an allowed prefix and sets the User-Agent", func() {
+			c := httpclient.NewExternal(time.Second, netip.MustParsePrefix("127.0.0.0/8"))
+			resp, err := c.Get(server.URL)
+			Expect(err).ToNot(HaveOccurred())
+			resp.Body.Close()
+			Expect(receivedUA).To(Equal(consts.HTTPUserAgent))
+		})
+	})
+
 	Describe("NewTransport", func() {
 		It("uses the default transport when base is nil", func() {
 			c := &http.Client{Transport: httpclient.NewTransport(nil)}

utils/netguard/netguard.go

@@ -0,0 +1,38 @@
+// Package netguard keeps outbound connections driven by untrusted input away from internal addresses.
+package netguard
+
+import (
+	"errors"
+	"fmt"
+	"net"
+	"net/netip"
+	"syscall"
+)
+
+var ErrPrivateAddress = errors.New("dial to private/loopback address blocked")
+
+// IsPrivateIP reports whether ip is loopback, private, link-local or unspecified.
+func IsPrivateIP(ip net.IP) bool {
+	return ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast()
+}
+
+// DialControl returns a net.Dialer Control hook that rejects private addresses not covered by allowed.
+// It sees the resolved IP, so DNS names, redirects and rebinding cannot get around it.
+func DialControl(allowed ...netip.Prefix) func(network, address string, c syscall.RawConn) error {
+	return func(_, address string, _ syscall.RawConn) error {
+		ap, err := netip.ParseAddrPort(address)
+		if err != nil {
+			return fmt.Errorf("%w: unparseable address %q: %w", ErrPrivateAddress, address, err)
+		}
+		addr := ap.Addr().Unmap()
+		if !IsPrivateIP(addr.AsSlice()) {
+			return nil
+		}
+		for _, p := range allowed {
+			if p.Contains(addr) {
+				return nil
+			}
+		}
+		return fmt.Errorf("%w: %s", ErrPrivateAddress, address)
+	}
+}

utils/netguard/netguard_suite_test.go

@@ -0,0 +1,17 @@
+package netguard_test
+
+import (
+	"testing"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestNetguard(t *testing.T) {
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Netguard Suite")
+}

utils/netguard/netguard_test.go

@@ -0,0 +1,65 @@
+package netguard_test
+
+import (
+	"net"
+	"net/netip"
+
+	"github.com/navidrome/navidrome/utils/netguard"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("netguard", func() {
+	DescribeTable("IsPrivateIP",
+		func(addr string, expected bool) {
+			Expect(netguard.IsPrivateIP(net.ParseIP(addr))).To(Equal(expected))
+		},
+		Entry("IPv4 loopback", "127.0.0.1", true),
+		Entry("IPv4 loopback range", "127.0.0.2", true),
+		Entry("IPv6 loopback", "::1", true),
+		Entry("IPv4-mapped IPv6 loopback", "::ffff:127.0.0.1", true),
+		Entry("10.x", "10.1.2.3", true),
+		Entry("172.16.x", "172.16.0.1", true),
+		Entry("192.168.x", "192.168.1.10", true),
+		Entry("IPv6 unique local", "fd00::1", true),
+		Entry("link-local (cloud metadata)", "169.254.169.254", true),
+		Entry("IPv6 link-local", "fe80::1", true),
+		Entry("link-local multicast", "224.0.0.1", true),
+		Entry("IPv4 unspecified", "0.0.0.0", true),
+		Entry("IPv6 unspecified", "::", true),
+		Entry("public IPv4", "93.184.216.34", false),
+		Entry("172.32.x is outside the private block", "172.32.0.1", false),
+		Entry("public IPv6", "2606:4700:4700::1111", false),
+	)
+
+	Describe("DialControl", func() {
+		It("rejects private and loopback addresses", func() {
+			control := netguard.DialControl()
+			for _, addr := range []string{"127.0.0.1:80", "[::1]:443", "169.254.169.254:80", "10.0.0.1:8080", "0.0.0.0:80"} {
+				Expect(control("tcp", addr, nil)).To(MatchError(netguard.ErrPrivateAddress), addr)
+			}
+		})
+
+		It("allows public addresses", func() {
+			control := netguard.DialControl()
+			Expect(control("tcp", "93.184.216.34:443", nil)).To(Succeed())
+			Expect(control("tcp6", "[2606:4700:4700::1111]:443", nil)).To(Succeed())
+		})
+
+		It("allows private addresses covered by an allowed prefix, and only those", func() {
+			control := netguard.DialControl(netip.MustParsePrefix("127.0.0.1/32"))
+			Expect(control("tcp", "127.0.0.1:8080", nil)).To(Succeed())
+			Expect(control("tcp", "127.0.0.2:8080", nil)).To(MatchError(netguard.ErrPrivateAddress))
+		})
+
+		It("matches IPv4-mapped IPv6 addresses against IPv4 prefixes", func() {
+			control := netguard.DialControl(netip.MustParsePrefix("127.0.0.0/8"))
+			Expect(control("tcp", "[::ffff:127.0.0.1]:80", nil)).To(Succeed())
+		})
+
+		It("fails closed when the address is not an IP", func() {
+			Expect(netguard.DialControl()("tcp", "localhost:80", nil)).To(HaveOccurred())
+			Expect(netguard.DialControl()("tcp", "garbage", nil)).To(HaveOccurred())
+		})
+	})
+})