feat(net): trusted-proxy depth so real client IPs survive a proxy — #2453

Fixes the defect the operator spotted in #370 immediately after it shipped:
auth.ClientIP ignored X-Forwarded-For whenever RemoteAddr was public, so a
proxy on a public address — a separate host, or a CDN, i.e. anyone running
this publicly, since public means TLS means a proxy — recorded the PROXY for
every session. created_ip and last_ip were then always equal and the
"Address changed" signal could never fire. The feature looked like it worked
and reported nothing.

Replaced with the standard trusted-hop model (Rails, Caddy, Traefik, nginx).
XFF grows left-to-right as each proxy appends the peer it received from, so
for client -> CDN -> own-proxy -> app the app sees [client, CDN] with
RemoteAddr = own-proxy, and the client sits at XFF[len - hops]:

  0  RemoteAddr, XFF ignored — no proxy
  1  the address your own proxy observed
  2  through a CDN in front of your proxy

Default 1, per the operator: publicly reachable means a TLS terminator in
front.

The cost is real and stated rather than hidden. hops >= 1 DECLARES that a
proxy exists; set it with no proxy, or deeper than the actual chain, and the
index reaches attacker-supplied entries, letting a visitor choose which
address their own session shows — defeating exactly the detection #370 is
for. That's inherent to the model, which is why 0 is a first-class value and
the admin card says "count your proxies, don't guess high" instead of just
exposing a number. Both mis-set shapes are pinned by tests so they stay known
consequences rather than surprises.

Migration 0053 + internal/netsettings, cached under an RWMutex. That's not an
optimisation: ClientIP runs in RequireUser for every authenticated request, so
a per-request query would put the database on the critical path of the whole
API. New() always returns a usable service so a boot-time DB hiccup degrades
to the default instead of breaking that path (rule #131), and Hops() is
nil-safe because test routers construct middleware without it.

RequireUser now takes a func() int rather than an int — the value is
operator-editable at runtime while the middleware is built once at boot, and
reading it per request is what makes a save take effect with no restart
(rule #25).

The admin card is verifiable, not just configurable: it reports the address
the CURRENT setting resolves THIS request to, the raw forwarded chain, and the
socket peer — so you set the number, save, and confirm the address matches the
machine you're on. It also counts the arriving chain and says how many proxies
that implies. GET/PUT both return that payload, PUT recomputed under the new
value, so the effect is visible without a reload.

Also fixes styling in the #370 card that CI could not catch: text-destructive
and bg-destructive don't exist in this Tailwind config — the palette is
colors.action.destructive — so the "Address changed" warning and the
sign-out-others button were rendering unstyled. Both now use
text-action-destructive / bg-action-destructive / text-action-fg.

Not done here: requestlog.go still logs raw RemoteAddr and will disagree with
the sessions UI about who connected. Left for its own change.

internal/api/admin_network.go

@@ -0,0 +1,65 @@
+package api
+
+import (
+	"encoding/json"
+	"errors"
+	"net/http"
+
+	"git.fabledsword.com/bvandeusen/minstrel/internal/apierror"
+	"git.fabledsword.com/bvandeusen/minstrel/internal/auth"
+	"git.fabledsword.com/bvandeusen/minstrel/internal/netsettings"
+)
+
+type networkSettingsResp struct {
+	TrustedProxyHops int `json:"trusted_proxy_hops"`
+	MaxHops          int `json:"max_hops"`
+	// DetectedClientIP is what the CURRENT setting resolves this very request
+	// to. It's the difference between a number the operator has to reason
+	// about and one they can verify: set the value, reload, and check the
+	// address matches the machine you're sitting at.
+	DetectedClientIP string `json:"detected_client_ip"`
+	// ForwardedChain is the raw X-Forwarded-For as received, so an operator
+	// whose detected address looks wrong can see how many hops actually
+	// arrived and count them rather than guess.
+	ForwardedChain string `json:"forwarded_chain"`
+	RemoteAddr     string `json:"remote_addr"`
+}
+
+type updateNetworkSettingsReq struct {
+	TrustedProxyHops int `json:"trusted_proxy_hops"`
+}
+
+func (h *handlers) handleGetNetworkSettings(w http.ResponseWriter, r *http.Request) {
+	writeJSON(w, http.StatusOK, h.networkSettingsPayload(r))
+}
+
+func (h *handlers) handleUpdateNetworkSettings(w http.ResponseWriter, r *http.Request) {
+	var req updateNetworkSettingsReq
+	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+		writeErr(w, apierror.BadRequest("invalid_body", "malformed JSON"))
+		return
+	}
+	if err := h.netSettings.SetHops(r.Context(), req.TrustedProxyHops); err != nil {
+		if errors.Is(err, netsettings.ErrHopsOutOfRange) {
+			writeErr(w, apierror.BadRequest("invalid_hops", err.Error()))
+			return
+		}
+		writeErrWithLog(w, h.logger, "admin network: update failed", apierror.Internal(err))
+		return
+	}
+	// Echo the payload recomputed under the NEW value, so the card can show
+	// immediately what the change did to this request's own address rather
+	// than making the operator reload to find out.
+	writeJSON(w, http.StatusOK, h.networkSettingsPayload(r))
+}
+
+func (h *handlers) networkSettingsPayload(r *http.Request) networkSettingsResp {
+	hops := h.netSettings.Hops()
+	return networkSettingsResp{
+		TrustedProxyHops: hops,
+		MaxHops:          netsettings.MaxTrustedProxyHops,
+		DetectedClientIP: auth.ClientIP(r, hops),
+		ForwardedChain:   r.Header.Get("X-Forwarded-For"),
+		RemoteAddr:       r.RemoteAddr,
+	}
+}

internal/api/api.go

@@ -20,6 +20,7 @@ import (
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/mailer"
+	"git.fabledsword.com/bvandeusen/minstrel/internal/netsettings"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/playlists"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/recsettings"
@@ -30,7 +31,7 @@ import (
 // Mount attaches /api/* handlers to r. Public endpoints (login) are outside
 // RequireUser; everything else is gated by the middleware. The events writer
 // is shared with the Subsonic mount so /rest/scrobble feeds the same store.
-func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte) {
+func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, lidarrCfg *lidarrconfig.Service, lidarrReqs *lidarrrequests.Service, lidarrQuar *lidarrquarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte, netSettings *netsettings.Service) {
 	rng := rand.New(rand.NewSource(rand.Int63()))
 	h := &handlers{
 		pool: pool, logger: logger, events: events, recCfg: recCfg,
@@ -51,6 +52,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
 		eventbus:          bus,
 		playlistScheduler: playlistScheduler,
 		streamSecret:      streamSecret,
+		netSettings:       netSettings,
 	}
 
 	r.Route("/api", func(api chi.Router) {
@@ -74,7 +76,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
 		api.With(auth.OptionalUser(pool, logger)).Get("/tracks/{id}/stream.{ext}", h.handleGetStream)
 
 		api.Group(func(authed chi.Router) {
-			authed.Use(auth.RequireUser(pool))
+			authed.Use(auth.RequireUser(pool, netSettings.Hops))
 			authed.Post("/auth/logout", h.handleLogout)
 			authed.Get("/me", h.handleGetMe)
 			authed.Get("/me/system-playlists-status", h.handleGetSystemPlaylistsStatus)
@@ -185,6 +187,9 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev
 				admin.Post("/albums/{id}/cover/refetch", h.handleAdminAlbumRefetchCover)
 				admin.Post("/covers/refetch-missing", h.handleAdminBulkRefetchCovers)
 
+				admin.Get("/network-settings", h.handleGetNetworkSettings)
+				admin.Put("/network-settings", h.handleUpdateNetworkSettings)
+
 				admin.Get("/scan/status", h.handleGetScanStatus)
 				admin.Post("/scan/run", h.handleTriggerScan)
 
@@ -264,6 +269,9 @@ type handlers struct {
 	mailer            mailer.Sender
 	eventbus          *eventbus.Bus
 	playlistScheduler *playlists.Scheduler
+	// netSettings caches the trusted reverse-proxy depth read by the auth
+	// middleware on every request and edited from the admin network card.
+	netSettings *netsettings.Service
 	// streamSecret is the HMAC key used by SignStreamToken /
 	// VerifyStreamToken to authenticate the UPnP-speaker stream path
 	// (see internal/api/stream_token.go and the design at

internal/auth/clientip.go

@@ -6,51 +6,101 @@ import (
 	"strings"
 )
 
-// ClientIP returns the caller's address for the active-sessions surface (#370).
+// ClientIP returns the caller's address, reading through trustedProxyHops
+// reverse proxies (#2453).
 //
-// Both obvious implementations are wrong, and they're wrong in ways that
-// matter specifically because this feeds a compromise-detection UI:
+// X-Forwarded-For grows left-to-right: every proxy APPENDS the peer it
+// received the request from. For client -> CDN -> own-proxy -> Minstrel the
+// app sees XFF = [client, CDN] and RemoteAddr = own-proxy. Each trusted proxy
+// therefore accounts for one entry counting from the right, and the first
+// address we were NOT told to trust is the client:
 //
-//   - r.RemoteAddr alone. Minstrel is normally behind a reverse proxy, so
-//     every session would show the proxy's address — noise shaped like data,
-//     hiding the exact thing the operator is looking for.
-//   - Trusting X-Forwarded-For. Any client can set that header, so an
-//     attacker could choose what appears in their victim's session list.
-//     A security surface an attacker can write to is worse than none.
+//	hops 0  -> RemoteAddr; XFF ignored entirely
+//	hops 1  -> XFF[1] = CDN — trusting only our own proxy, the most we can
+//	           honestly claim is the address it told us about
+//	hops 2  -> XFF[0] = client
 //
-// So the header is trusted only when the request actually arrived from a
-// proxy. If RemoteAddr is public, the caller reached us directly and its XFF
-// is attacker-controlled, so it's ignored outright. If RemoteAddr is
-// private/loopback, XFF is walked from the RIGHT — entries are appended as a
-// request passes through infrastructure, so the rightmost end is the one our
-// own proxies wrote — and the first address that isn't itself a proxy range
-// wins. A client forging XFF can only prepend to the untrusted left end,
-// which that walk never reaches.
+// This replaces an earlier heuristic that ignored XFF whenever RemoteAddr was
+// public. That was safe but useless in the deployment that matters: a proxy
+// on a public address (separate host, or a CDN) meant every session recorded
+// the proxy, so the active-sessions surface could never show an address
+// change (#370).
 //
-// Known limitation, failing closed on purpose: if the proxy sits on a PUBLIC
-// address (a separate host, or a CDN in front), RemoteAddr isn't in a proxy
-// range, so we report the proxy rather than the end user. That's a true fact
-// about where the request came from, which beats trusting a forgeable header.
+// # What the operator is asserting
 //
-// Returns "" when nothing usable can be determined. Callers store that as-is
-// and the UI renders "unknown" rather than inventing a value.
-func ClientIP(r *http.Request) string {
+// hops >= 1 is a DECLARATION that a proxy sits in front. Two ways to get it
+// wrong, both worth understanding rather than papering over:
+//
+//   - Set to 1+ with NO proxy: any client can forge X-Forwarded-For and pick
+//     what its own session row shows, defeating the compromise detection.
+//   - Set HIGHER than the real chain: the index runs past the proxy-written
+//     entries into attacker-supplied ones, same result.
+//
+// Both are inherent to the trusted-hop model — Rails, Caddy, Traefik and
+// nginx all behave this way — which is why 0 is a first-class value and the
+// admin card tells the operator to count their proxies.
+func ClientIP(r *http.Request, trustedProxyHops int) string {
 	remote := hostOf(r.RemoteAddr)
-	ip := net.ParseIP(remote)
-	if ip == nil || !isProxyRange(ip) {
+	if trustedProxyHops <= 0 {
 		return remote
 	}
-	if forwarded := forwardedClient(r.Header.Get("X-Forwarded-For")); forwarded != "" {
-		return forwarded
+	chain := forwardedChain(r)
+	if len(chain) == 0 {
+		// No forwarding header: either there's genuinely no proxy, or one is
+		// misconfigured. The socket peer is the only thing we actually know.
+		return remote
 	}
-	// Some proxies set only X-Real-IP. The trust condition is already
-	// satisfied — we know this request came from a proxy range.
-	if real := net.ParseIP(strings.TrimSpace(r.Header.Get("X-Real-IP"))); real != nil {
-		return real.String()
+	// Clamp rather than reject: a chain shorter than the configured depth
+	// means the operator over-counted, and the leftmost entry is the closest
+	// thing to a client on offer. The caveat above covers the risk.
+	idx := len(chain) - trustedProxyHops
+	if idx < 0 {
+		idx = 0
 	}
+	if ip := net.ParseIP(chain[idx]); ip != nil {
+		return ip.String()
+	}
+	// A proxy wrote something that isn't an address. Positional meaning is
+	// lost, so fall back to what we can verify ourselves.
 	return remote
 }
 
+// forwardedChain returns the X-Forwarded-For entries in wire order, or the
+// single X-Real-IP value when XFF is absent.
+//
+// Entries are kept verbatim, including unparseable ones: their POSITION is
+// what carries meaning here, so silently dropping a malformed hop would
+// shift every index and could hand back an attacker-supplied entry.
+func forwardedChain(r *http.Request) []string {
+	raw := r.Header.Get("X-Forwarded-For")
+	if strings.TrimSpace(raw) == "" {
+		// Some proxies set only X-Real-IP, which by construction is a single
+		// hop — the address that proxy saw.
+		if real := strings.TrimSpace(r.Header.Get("X-Real-IP")); real != "" {
+			return []string{real}
+		}
+		return nil
+	}
+	parts := strings.Split(raw, ",")
+	out := make([]string, 0, len(parts))
+	for _, p := range parts {
+		if p = strings.TrimSpace(p); p != "" {
+			out = append(out, p)
+		}
+	}
+	return out
+}
+
+// hopsOf reads a trusted-depth accessor, treating a nil one as "trust
+// nothing". Test contexts and any future caller that hasn't wired the
+// settings service get the safe reading rather than a panic.
+func hopsOf(fn func() int) int {
+	if fn == nil {
+		return 0
+	}
+	return fn()
+}
+
 // hostOf strips the port from a RemoteAddr, tolerating values that have none.
 func hostOf(remoteAddr string) string {
 	host, _, err := net.SplitHostPort(remoteAddr)
@@ -59,34 +109,3 @@ func hostOf(remoteAddr string) string {
 	}
 	return host
 }
-
-// forwardedClient walks an X-Forwarded-For value right-to-left and returns
-// the first address outside our proxy ranges — see ClientIP for why the
-// direction matters. Returns "" if the header is absent, malformed, or
-// contains nothing but proxy addresses.
-func forwardedClient(header string) string {
-	parts := strings.Split(header, ",")
-	for i := len(parts) - 1; i >= 0; i-- {
-		ip := net.ParseIP(strings.TrimSpace(parts[i]))
-		if ip == nil || isProxyRange(ip) {
-			continue
-		}
-		return ip.String()
-	}
-	return ""
-}
-
-// isProxyRange reports whether ip is an address a reverse proxy would
-// plausibly occupy in a self-hosted deployment: loopback, RFC1918 / ULA
-// (both covered by IsPrivate), link-local, or unspecified.
-//
-// Deliberately not configurable. These ranges cover proxy-on-same-host and
-// proxy-on-the-same-docker-network, which is essentially every self-hosted
-// install, and it works with no setup at all (rule #26). An exotic topology
-// can motivate a setting when one actually turns up.
-func isProxyRange(ip net.IP) bool {
-	return ip.IsLoopback() ||
-		ip.IsPrivate() ||
-		ip.IsLinkLocalUnicast() ||
-		ip.IsUnspecified()
-}

internal/auth/clientip_test.go

@@ -5,99 +5,148 @@ import (
 	"testing"
 )
 
-// The spoofing cases below are the reason this function exists rather than a
-// one-line r.RemoteAddr read, so they're asserted explicitly rather than
-// folded into the happy-path table.
+// The hop arithmetic is the whole feature, so the table is written as
+// deployment topologies rather than abstract inputs.
 func TestClientIP(t *testing.T) {
 	tests := []struct {
 		name       string
+		hops       int
 		remoteAddr string
 		forwarded  string
 		realIP     string
 		want       string
 	}{
 		{
-			name:       "direct connection, no proxy headers",
+			name:       "no proxy configured, socket peer wins",
+			hops:       0,
 			remoteAddr: "203.0.113.5:51234",
 			want:       "203.0.113.5",
 		},
 		{
-			// The attack this guards against: a client connecting straight to
-			// us claims to be someone else. RemoteAddr is public, so it did
-			// NOT come through our proxy, so its XFF is worthless.
-			name:       "direct connection ignores forged X-Forwarded-For",
+			// hops 0 is the setting for a directly-exposed instance, and it
+			// must make forged headers inert.
+			name:       "hops 0 ignores a forged forwarded header",
+			hops:       0,
 			remoteAddr: "203.0.113.5:51234",
 			forwarded:  "198.51.100.99",
 			want:       "203.0.113.5",
 		},
 		{
-			name:       "direct connection ignores forged X-Real-IP",
+			// The common case: one TLS-terminating proxy. Note RemoteAddr is
+			// PUBLIC here — a proxy on its own host — which the previous
+			// private-range heuristic got wrong.
+			name:       "one proxy on a public address yields the client",
+			hops:       1,
+			remoteAddr: "203.0.113.200:40000",
+			forwarded:  "198.51.100.7",
+			want:       "198.51.100.7",
+		},
+		{
+			name:       "one proxy on a private address yields the client",
+			hops:       1,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "198.51.100.7",
+			want:       "198.51.100.7",
+		},
+		{
+			// client -> Cloudflare -> own proxy -> app.
+			// Trusting only our own proxy, the honest answer is Cloudflare:
+			// that's the address our proxy actually observed.
+			name:       "cdn chain with hops 1 stops at the cdn",
+			hops:       1,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "198.51.100.7, 203.0.113.50",
+			want:       "203.0.113.50",
+		},
+		{
+			// Same chain, both hops trusted — now we reach the real client.
+			name:       "cdn chain with hops 2 reaches the client",
+			hops:       2,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "198.51.100.7, 203.0.113.50",
+			want:       "198.51.100.7",
+		},
+		{
+			// A client prepending a lie is only reachable if the operator
+			// over-counts their proxies; at the correct depth it's skipped.
+			name:       "forged prefix is not reached at the correct depth",
+			hops:       1,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "1.2.3.4, 198.51.100.7",
+			want:       "198.51.100.7",
+		},
+		{
+			// The documented mis-set failure, pinned so it stays a KNOWN
+			// consequence rather than a surprise: depth deeper than the real
+			// chain reads attacker-supplied input.
+			name:       "hops set deeper than the chain clamps to the leftmost entry",
+			hops:       5,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "1.2.3.4, 198.51.100.7",
+			want:       "1.2.3.4",
+		},
+		{
+			name:       "no forwarding header falls back to the socket peer",
+			hops:       1,
 			remoteAddr: "203.0.113.5:51234",
-			realIP:     "198.51.100.99",
 			want:       "203.0.113.5",
 		},
 		{
-			name:       "behind proxy, single forwarded client",
+			name:       "x-real-ip used when forwarded-for is absent",
+			hops:       1,
 			remoteAddr: "172.18.0.1:40000",
-			forwarded:  "203.0.113.5",
-			want:       "203.0.113.5",
+			realIP:     "198.51.100.7",
+			want:       "198.51.100.7",
 		},
 		{
-			// A client that prepends a lie to XFF only pollutes the LEFT end;
-			// the proxy appends the address it actually saw on the right. The
-			// right-to-left walk reaches the truth first.
-			name:       "behind proxy, forged prefix is skipped for the appended truth",
-			remoteAddr: "10.0.0.2:40000",
-			forwarded:  "198.51.100.99, 203.0.113.5",
-			want:       "203.0.113.5",
-		},
-		{
-			name:       "behind proxy chain, internal hops skipped",
-			remoteAddr: "10.0.0.2:40000",
-			forwarded:  "203.0.113.5, 10.0.0.7, 172.18.0.3",
-			want:       "203.0.113.5",
-		},
-		{
-			name:       "behind proxy, X-Real-IP used when no forwarded header",
-			remoteAddr: "127.0.0.1:40000",
-			realIP:     "203.0.113.5",
-			want:       "203.0.113.5",
-		},
-		{
-			// LAN client through a LAN proxy: everything is private, so there
-			// is no public address to find. Reporting the peer is honest.
-			name:       "behind proxy, all-private chain falls back to remote",
+			name:       "forwarded-for wins over x-real-ip when both present",
+			hops:       1,
 			remoteAddr: "172.18.0.1:40000",
-			forwarded:  "192.168.1.50, 172.18.0.3",
+			forwarded:  "198.51.100.7",
+			realIP:     "1.2.3.4",
+			want:       "198.51.100.7",
+		},
+		{
+			// Positions are preserved, so a garbage hop can be selected —
+			// in which case we fall back rather than return nonsense.
+			name:       "unparseable selected entry falls back to the socket peer",
+			hops:       1,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "198.51.100.7, not-an-ip",
 			want:       "172.18.0.1",
 		},
 		{
-			name:       "behind proxy, malformed forwarded entries ignored",
-			remoteAddr: "172.18.0.1:40000",
-			forwarded:  "not-an-ip, 203.0.113.5, also-garbage",
-			want:       "203.0.113.5",
-		},
-		{
-			name:       "remote addr without a port is tolerated",
-			remoteAddr: "203.0.113.5",
-			want:       "203.0.113.5",
-		},
-		{
-			name:       "ipv6 remote addr",
-			remoteAddr: "[2001:db8::1]:51234",
-			want:       "2001:db8::1",
-		},
-		{
-			name:       "ipv6 forwarded client behind proxy",
+			name:       "ipv6 client through one proxy",
+			hops:       1,
 			remoteAddr: "[fd00::1]:40000",
 			forwarded:  "2001:db8::5",
 			want:       "2001:db8::5",
 		},
+		{
+			name:       "ipv6 socket peer without proxy",
+			hops:       0,
+			remoteAddr: "[2001:db8::1]:51234",
+			want:       "2001:db8::1",
+		},
+		{
+			name:       "remote addr without a port is tolerated",
+			hops:       0,
+			remoteAddr: "203.0.113.5",
+			want:       "203.0.113.5",
+		},
 		{
 			name:       "empty remote addr yields empty",
+			hops:       1,
 			remoteAddr: "",
 			want:       "",
 		},
+		{
+			name:       "whitespace-only forwarded header is treated as absent",
+			hops:       1,
+			remoteAddr: "172.18.0.1:40000",
+			forwarded:  "   ",
+			want:       "172.18.0.1",
+		},
 	}
 
 	for _, tc := range tests {
@@ -113,8 +162,8 @@ func TestClientIP(t *testing.T) {
 			if tc.realIP != "" {
 				r.Header.Set("X-Real-IP", tc.realIP)
 			}
-			if got := ClientIP(r); got != tc.want {
-				t.Errorf("ClientIP() = %q, want %q", got, tc.want)
+			if got := ClientIP(r, tc.hops); got != tc.want {
+				t.Errorf("ClientIP(hops=%d) = %q, want %q", tc.hops, got, tc.want)
 			}
 		})
 	}

internal/auth/session.go

@@ -56,7 +56,13 @@ const SessionCookieName = "minstrel_session"
 // bearer header and puts the dbq.User in request context via userCtxKey.
 // Requests without a valid session return 401 with no body so callers don't
 // leak whether the username existed (matches the /rest/* auth posture).
-func RequireUser(pool *pgxpool.Pool) func(http.Handler) http.Handler {
+//
+// trustedHops supplies the reverse-proxy depth used to record the session's
+// current address (#2453). It's a func rather than an int because the value
+// is operator-editable at runtime and this middleware is constructed once at
+// boot — reading it per request is what makes an admin change take effect
+// without a restart. Passing nil means "trust nothing", i.e. the socket peer.
+func RequireUser(pool *pgxpool.Pool, trustedHops func() int) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			token := sessionTokenFromRequest(r)
@@ -103,7 +109,7 @@ func RequireUser(pool *pgxpool.Pool) func(http.Handler) http.Handler {
 			// surface exists to show, and it costs nothing extra here.
 			if err := q.TouchSessionLastSeen(r.Context(), dbq.TouchSessionLastSeenParams{
 				ID:     sess.ID,
-				LastIp: ClientIP(r),
+				LastIp: ClientIP(r, hopsOf(trustedHops)),
 			}); err != nil {
 				slog.Warn("api: touch session last_seen failed", "err", err)
 			}

internal/auth/session_test.go

@@ -57,7 +57,7 @@ func TestRequireUser_RejectsWhenNoCookieOrBearer(t *testing.T) {
 	next := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
 		t.Fatal("handler must not be called")
 	})
-	h := RequireUser(nil)(next)
+	h := RequireUser(nil, nil)(next)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/me", nil)
 	w := httptest.NewRecorder()

internal/db/dbq/models.go

@@ -381,6 +381,11 @@ type LidarrRequest struct {
 	LidarrAddConfirmedAt pgtype.Timestamptz
 }
 
+type NetworkSetting struct {
+	ID               bool
+	TrustedProxyHops int32
+}
+
 type PasswordReset struct {
 	Token     string
 	UserID    pgtype.UUID

internal/db/dbq/network_settings.sql.go

@@ -0,0 +1,32 @@
+// Code generated by sqlc. DO NOT EDIT.
+// versions:
+//   sqlc v1.31.1
+// source: network_settings.sql
+
+package dbq
+
+import (
+	"context"
+)
+
+const getNetworkSettings = `-- name: GetNetworkSettings :one
+SELECT id, trusted_proxy_hops FROM network_settings WHERE id = true
+`
+
+func (q *Queries) GetNetworkSettings(ctx context.Context) (NetworkSetting, error) {
+	row := q.db.QueryRow(ctx, getNetworkSettings)
+	var i NetworkSetting
+	err := row.Scan(&i.ID, &i.TrustedProxyHops)
+	return i, err
+}
+
+const updateTrustedProxyHops = `-- name: UpdateTrustedProxyHops :one
+UPDATE network_settings SET trusted_proxy_hops = $1 WHERE id = true RETURNING id, trusted_proxy_hops
+`
+
+func (q *Queries) UpdateTrustedProxyHops(ctx context.Context, trustedProxyHops int32) (NetworkSetting, error) {
+	row := q.db.QueryRow(ctx, updateTrustedProxyHops, trustedProxyHops)
+	var i NetworkSetting
+	err := row.Scan(&i.ID, &i.TrustedProxyHops)
+	return i, err
+}

internal/db/migrations/0053_network_settings.down.sql

@@ -0,0 +1 @@
+DROP TABLE network_settings;

internal/db/migrations/0053_network_settings.up.sql

@@ -0,0 +1,33 @@
+-- Trusted reverse-proxy depth for client-IP extraction (#2453).
+--
+-- X-Forwarded-For grows left-to-right: each proxy APPENDS the peer it
+-- received the request from. For client -> CDN -> own-proxy -> Minstrel the
+-- app sees XFF = [client, CDN] with RemoteAddr = own-proxy. So the real
+-- client sits at XFF[len - hops], where hops counts the proxies you trust:
+--
+--   0  no proxy in front — use the socket peer, ignore XFF entirely
+--   1  one reverse proxy (nginx / Caddy / Traefik terminating TLS)
+--   2  a CDN in front of your own proxy (Cloudflare -> nginx -> Minstrel)
+--
+-- Default 1: a publicly reachable Minstrel needs a TLS terminator in front of
+-- it, and recording that terminator's own address for every session makes the
+-- active-sessions surface (#370) useless — created_ip and last_ip would both
+-- be the proxy, so the "address changed" signal could never fire.
+--
+-- The cost, stated on the admin card rather than buried: hops >= 1 DECLARES
+-- that a proxy exists. If one doesn't, a client can forge X-Forwarded-For and
+-- choose what its own session row shows, which defeats exactly the compromise
+-- detection #370 exists for. That is inherent to the trusted-hop model, which
+-- is why 0 is a first-class setting and not a hidden escape hatch.
+--
+-- Upper bound 10 guards a typo turning into "trust the whole header"; no real
+-- deployment chains ten proxies.
+CREATE TABLE network_settings (
+    id                 boolean PRIMARY KEY DEFAULT true,
+    trusted_proxy_hops int NOT NULL DEFAULT 1,
+    CONSTRAINT network_settings_singleton CHECK (id = true),
+    CONSTRAINT network_settings_hops_range
+        CHECK (trusted_proxy_hops >= 0 AND trusted_proxy_hops <= 10)
+);
+
+INSERT INTO network_settings (id) VALUES (true) ON CONFLICT (id) DO NOTHING;

internal/db/queries/network_settings.sql

@@ -0,0 +1,5 @@
+-- name: GetNetworkSettings :one
+SELECT * FROM network_settings WHERE id = true;
+
+-- name: UpdateTrustedProxyHops :one
+UPDATE network_settings SET trusted_proxy_hops = $1 WHERE id = true RETURNING *;

internal/netsettings/service.go

@@ -0,0 +1,98 @@
+// Package netsettings holds the DB-backed network settings the request path
+// needs. Today that's the trusted reverse-proxy depth used to pull a real
+// client address out of X-Forwarded-For (#2453).
+//
+// Values are cached under an RWMutex and refreshed on write. That isn't an
+// optimisation: auth.ClientIP runs in the RequireUser middleware for every
+// authenticated request, so a per-request query here would put the database
+// on the critical path of the entire API.
+package netsettings
+
+import (
+	"context"
+	"errors"
+	"log/slog"
+	"sync"
+
+	"github.com/jackc/pgx/v5/pgxpool"
+
+	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
+)
+
+const (
+	// DefaultTrustedProxyHops mirrors migration 0053's column default. One
+	// proxy, because anything publicly reachable needs a TLS terminator in
+	// front of it.
+	DefaultTrustedProxyHops = 1
+	// MaxTrustedProxyHops mirrors the CHECK in migration 0053.
+	MaxTrustedProxyHops = 10
+)
+
+// ErrHopsOutOfRange is returned by SetHops for values the CHECK would reject,
+// so the API layer can answer 400 instead of surfacing a constraint violation.
+var ErrHopsOutOfRange = errors.New("trusted proxy hops must be between 0 and 10")
+
+// Service caches the network settings and owns their persistence.
+type Service struct {
+	pool   *pgxpool.Pool
+	logger *slog.Logger
+
+	mu   sync.RWMutex
+	hops int
+}
+
+// New loads the settings once and caches them.
+//
+// It ALWAYS returns a usable Service, even alongside a non-nil error. The
+// value it holds sits on the authenticated request path, so a boot-time
+// database hiccup must degrade to the default rather than take every request
+// down with it (rule #131). The error is returned so the caller can log that
+// the cache holds a default rather than stored state.
+func New(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) (*Service, error) {
+	s := &Service{pool: pool, logger: logger, hops: DefaultTrustedProxyHops}
+	if pool == nil {
+		return s, nil
+	}
+	row, err := dbq.New(pool).GetNetworkSettings(ctx)
+	if err != nil {
+		return s, err
+	}
+	s.hops = int(row.TrustedProxyHops)
+	return s, nil
+}
+
+// Hops returns the cached trusted-proxy depth.
+//
+// Nil-safe: test contexts construct routers without this service, and a
+// missing setting should mean "trust nothing" rather than a panic in
+// middleware.
+func (s *Service) Hops() int {
+	if s == nil {
+		return 0
+	}
+	s.mu.RLock()
+	defer s.mu.RUnlock()
+	return s.hops
+}
+
+// SetHops persists a new depth and refreshes the cache, so an admin change
+// takes effect on the next request with no restart (rule #25).
+func (s *Service) SetHops(ctx context.Context, hops int) error {
+	if s == nil || s.pool == nil {
+		// Mirrors Hops()'s nil-tolerance: handlers can be constructed without
+		// this service in tests, and a write attempt there should be an error
+		// rather than a panic in an HTTP handler.
+		return errors.New("network settings unavailable")
+	}
+	if hops < 0 || hops > MaxTrustedProxyHops {
+		return ErrHopsOutOfRange
+	}
+	row, err := dbq.New(s.pool).UpdateTrustedProxyHops(ctx, int32(hops))
+	if err != nil {
+		return err
+	}
+	s.mu.Lock()
+	s.hops = int(row.TrustedProxyHops)
+	s.mu.Unlock()
+	return nil
+}

internal/netsettings/service_test.go

@@ -0,0 +1,55 @@
+package netsettings
+
+import (
+	"context"
+	"errors"
+	"testing"
+)
+
+// A nil service reaches middleware in test routers and anywhere the settings
+// aren't wired. It must read as "trust nothing" rather than panic — the
+// alternative is a nil dereference inside RequireUser, on every request.
+func TestHops_NilServiceTrustsNothing(t *testing.T) {
+	var s *Service
+	if got := s.Hops(); got != 0 {
+		t.Errorf("(*Service)(nil).Hops() = %d, want 0", got)
+	}
+}
+
+func TestNew_NilPoolYieldsDefault(t *testing.T) {
+	s, err := New(context.Background(), nil, nil)
+	if err != nil {
+		t.Fatalf("New with nil pool: %v", err)
+	}
+	if s == nil {
+		t.Fatal("New returned nil service")
+	}
+	if got := s.Hops(); got != DefaultTrustedProxyHops {
+		t.Errorf("Hops() = %d, want %d", got, DefaultTrustedProxyHops)
+	}
+}
+
+// Range is rejected before the query so the API answers 400 rather than
+// surfacing a CHECK violation as a 500.
+func TestSetHops_RejectsOutOfRange(t *testing.T) {
+	s, _ := New(context.Background(), nil, nil)
+	for _, hops := range []int{-1, MaxTrustedProxyHops + 1, 999} {
+		if err := s.SetHops(context.Background(), hops); !errors.Is(err, ErrHopsOutOfRange) {
+			t.Errorf("SetHops(%d) error = %v, want ErrHopsOutOfRange", hops, err)
+		}
+	}
+}
+
+// In-range values with no pool must still fail, and must not mutate the
+// cache — a write that didn't persist reporting success would leave the
+// running process disagreeing with the database.
+func TestSetHops_NoPoolFailsWithoutMutatingCache(t *testing.T) {
+	s, _ := New(context.Background(), nil, nil)
+	before := s.Hops()
+	if err := s.SetHops(context.Background(), 2); err == nil {
+		t.Error("SetHops with nil pool returned nil error")
+	}
+	if after := s.Hops(); after != before {
+		t.Errorf("cache changed from %d to %d despite a failed write", before, after)
+	}
+}

internal/server/server.go

@@ -25,6 +25,7 @@ import (
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/mailer"
+	"git.fabledsword.com/bvandeusen/minstrel/internal/netsettings"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/playevents"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/playlists"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/recsettings"
@@ -164,13 +165,21 @@ func (s *Server) Router() http.Handler {
 				s.Logger.Error("server: recsettings boot failed", "err", err)
 			}
 		}
-		api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, lidarrCfg, lidarrReqs, lidarrQuar, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret)
+		// Cached trusted-proxy depth (#2453). Constructed here rather than in
+		// main.go because nothing else needs it at boot, and New always hands
+		// back a usable service — a DB hiccup degrades to the default rather
+		// than breaking the authenticated request path that reads it.
+		netSettings, err := netsettings.New(context.Background(), s.Pool, s.Logger)
+		if err != nil {
+			s.Logger.Error("server: netsettings boot failed, using default hops", "err", err)
+		}
+		api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, lidarrCfg, lidarrReqs, lidarrQuar, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret, netSettings)
 		// /api/admin/scan is the only admin route owned by the server package
 		// (it needs the Scanner). Register it as a single inline-middleware
 		// route — using r.Route("/api/admin", ...) here would create a second
 		// subtree that shadows every admin route registered by api.Mount.
 		if s.Scanner != nil {
-			r.With(auth.RequireUser(s.Pool), auth.RequireAdmin()).
+			r.With(auth.RequireUser(s.Pool, netSettings.Hops), auth.RequireAdmin()).
 				Post("/api/admin/scan", s.handleAdminScan)
 		}
 		subsonic.Mount(r, s.Pool, s.Logger, s.SubsonicCfg, writer)