From 381e9cedb702667d331188889c32f68610428328 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 5 Aug 2026 10:07:43 -0400 Subject: [PATCH] =?UTF-8?q?feat(net):=20trusted-proxy=20depth=20so=20real?= =?UTF-8?q?=20client=20IPs=20survive=20a=20proxy=20=E2=80=94=20#2453?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 65 +++++++ internal/api/api.go | 12 +- internal/auth/clientip.go | 145 +++++++++------- internal/auth/clientip_test.go | 163 ++++++++++++------ internal/auth/session.go | 10 +- internal/auth/session_test.go | 2 +- internal/db/dbq/models.go | 5 + internal/db/dbq/network_settings.sql.go | 32 ++++ .../migrations/0053_network_settings.down.sql | 1 + .../migrations/0053_network_settings.up.sql | 33 ++++ internal/db/queries/network_settings.sql | 5 + internal/netsettings/service.go | 98 +++++++++++ internal/netsettings/service_test.go | 55 ++++++ internal/server/server.go | 13 +- web/src/lib/api/admin.ts | 25 +++ web/src/lib/components/ActiveSessions.svelte | 6 +- .../lib/components/NetworkSettingsCard.svelte | 135 +++++++++++++++ .../components/NetworkSettingsCard.test.ts | 106 ++++++++++++ .../routes/admin/integrations/+page.svelte | 7 + 19 files changed, 788 insertions(+), 130 deletions(-) create mode 100644 internal/api/admin_network.go create mode 100644 internal/db/dbq/network_settings.sql.go create mode 100644 internal/db/migrations/0053_network_settings.down.sql create mode 100644 internal/db/migrations/0053_network_settings.up.sql create mode 100644 internal/db/queries/network_settings.sql create mode 100644 internal/netsettings/service.go create mode 100644 internal/netsettings/service_test.go create mode 100644 web/src/lib/components/NetworkSettingsCard.svelte create mode 100644 web/src/lib/components/NetworkSettingsCard.test.ts diff --git a/internal/api/admin_network.go b/internal/api/admin_network.go new file mode 100644 index 00000000..4ab12d63 --- /dev/null +++ b/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, + } +} diff --git a/internal/api/api.go b/internal/api/api.go index d27e2e47..e9c481b5 100644 --- a/internal/api/api.go +++ b/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 diff --git a/internal/auth/clientip.go b/internal/auth/clientip.go index ba228fe0..2be913cb 100644 --- a/internal/auth/clientip.go +++ b/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() -} diff --git a/internal/auth/clientip_test.go b/internal/auth/clientip_test.go index 8b49633e..fba50fb9 100644 --- a/internal/auth/clientip_test.go +++ b/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) } }) } diff --git a/internal/auth/session.go b/internal/auth/session.go index 9ebed6e4..49eda982 100644 --- a/internal/auth/session.go +++ b/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) } diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go index e805e650..88d03984 100644 --- a/internal/auth/session_test.go +++ b/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() diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 56fc74af..51cdc693 100644 --- a/internal/db/dbq/models.go +++ b/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 diff --git a/internal/db/dbq/network_settings.sql.go b/internal/db/dbq/network_settings.sql.go new file mode 100644 index 00000000..89573cfd --- /dev/null +++ b/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 +} diff --git a/internal/db/migrations/0053_network_settings.down.sql b/internal/db/migrations/0053_network_settings.down.sql new file mode 100644 index 00000000..88c2e842 --- /dev/null +++ b/internal/db/migrations/0053_network_settings.down.sql @@ -0,0 +1 @@ +DROP TABLE network_settings; diff --git a/internal/db/migrations/0053_network_settings.up.sql b/internal/db/migrations/0053_network_settings.up.sql new file mode 100644 index 00000000..d53649ff --- /dev/null +++ b/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; diff --git a/internal/db/queries/network_settings.sql b/internal/db/queries/network_settings.sql new file mode 100644 index 00000000..9527590e --- /dev/null +++ b/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 *; diff --git a/internal/netsettings/service.go b/internal/netsettings/service.go new file mode 100644 index 00000000..0f062ab6 --- /dev/null +++ b/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 +} diff --git a/internal/netsettings/service_test.go b/internal/netsettings/service_test.go new file mode 100644 index 00000000..c01dd645 --- /dev/null +++ b/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) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 4b75e7b4..e46c0ee6 100644 --- a/internal/server/server.go +++ b/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) diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index 8fafa7a8..d1eb16c5 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -644,3 +644,28 @@ export function createDiagnosticDevicesQuery(userId?: string) { staleTime: 15_000 }); } + +// Trusted-proxy depth (#2453) --------------------------------------------- + +// detected_client_ip / forwarded_chain / remote_addr describe THIS request +// under the current setting, so the admin card can be verified rather than +// reasoned about: change the number, see what address you resolve to. +export type NetworkSettings = { + trusted_proxy_hops: number; + max_hops: number; + detected_client_ip: string; + forwarded_chain: string; + remote_addr: string; +}; + +export async function getNetworkSettings(): Promise { + return api.get('/api/admin/network-settings'); +} + +// Returns the payload recomputed under the new value, so the card can show +// the effect immediately instead of requiring a reload. +export async function updateNetworkSettings(hops: number): Promise { + return api.put('/api/admin/network-settings', { + trusted_proxy_hops: hops + }); +} diff --git a/web/src/lib/components/ActiveSessions.svelte b/web/src/lib/components/ActiveSessions.svelte index bcde8318..c7c0229f 100644 --- a/web/src/lib/components/ActiveSessions.svelte +++ b/web/src/lib/components/ActiveSessions.svelte @@ -127,7 +127,7 @@

{#if loadError} -

+

Couldn't load your sessions.

@@ -156,7 +156,7 @@ {/if} {#if hasMoved(s)} +

+ {:else if settings === null} +

Loading…

+ {:else} +
+ + +
+ + +
+
Your address right now
+
{settings.detected_client_ip || 'unknown'}
+
Direct connection from
+
{settings.remote_addr || 'unknown'}
+
Forwarded chain
+
{settings.forwarded_chain || '(none)'}
+
+ + {#if suggested > 0 && settings.trusted_proxy_hops !== suggested} +

+ This request arrived with {suggested} + {suggested === 1 ? 'forwarded address' : 'forwarded addresses'}, which usually means + {suggested} + {suggested === 1 ? 'proxy' : 'proxies'} in front of Minstrel. +

+ {/if} + +
+

+

+
    +
  • 0 — no proxy; Minstrel is reached directly.
    • +
  • 1 — one reverse proxy, e.g. nginx, Caddy or Traefik terminating TLS.
    • +
  • 2 — a CDN in front of your own proxy, e.g. Cloudflare → nginx.
    • +
+
+ {/if} + diff --git a/web/src/lib/components/NetworkSettingsCard.test.ts b/web/src/lib/components/NetworkSettingsCard.test.ts new file mode 100644 index 00000000..b7bc4eef --- /dev/null +++ b/web/src/lib/components/NetworkSettingsCard.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, test, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; +import NetworkSettingsCard from './NetworkSettingsCard.svelte'; + +const getNetworkSettings = vi.fn(); +const updateNetworkSettings = vi.fn(); + +vi.mock('$lib/api/admin', () => ({ + getNetworkSettings: () => getNetworkSettings(), + updateNetworkSettings: (hops: number) => updateNetworkSettings(hops) +})); + +vi.mock('$lib/stores/toast.svelte', () => ({ pushToast: vi.fn() })); + +function settings(over: Record = {}) { + return { + trusted_proxy_hops: 1, + max_hops: 10, + detected_client_ip: '198.51.100.7', + forwarded_chain: '198.51.100.7', + remote_addr: '172.18.0.1:40000', + ...over + }; +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe('NetworkSettingsCard', () => { + // The detected address is the card's verification affordance — the number + // is abstract, this is checkable against the machine you're sitting at. + test('shows the address the current setting resolves to', async () => { + getNetworkSettings.mockResolvedValue(settings()); + render(NetworkSettingsCard); + + expect(await screen.findByText('198.51.100.7')).toBeTruthy(); + expect(screen.getByText('172.18.0.1:40000')).toBeTruthy(); + }); + + test('save is inert until the value actually changes', async () => { + getNetworkSettings.mockResolvedValue(settings({ trusted_proxy_hops: 1 })); + render(NetworkSettingsCard); + + const save = await screen.findByRole('button', { name: /Save/ }); + expect(save).toBeDisabled(); + + const input = screen.getByRole('spinbutton'); + await fireEvent.input(input, { target: { value: '2' } }); + await waitFor(() => expect(save).not.toBeDisabled()); + }); + + test('saving sends the new depth and adopts the echoed value', async () => { + getNetworkSettings.mockResolvedValue(settings({ trusted_proxy_hops: 1 })); + updateNetworkSettings.mockResolvedValue( + settings({ trusted_proxy_hops: 2, detected_client_ip: '203.0.113.9' }) + ); + render(NetworkSettingsCard); + + const input = await screen.findByRole('spinbutton'); + await fireEvent.input(input, { target: { value: '2' } }); + await fireEvent.click(screen.getByRole('button', { name: /Save/ })); + + await waitFor(() => expect(updateNetworkSettings).toHaveBeenCalledWith(2)); + // The recomputed address proves the change took effect on this request. + expect(await screen.findByText('203.0.113.9')).toBeTruthy(); + }); + + // Counting proxies is the operator's job and the hint is how they do it + // without guessing. + test('hints the likely depth when it disagrees with the arriving chain', async () => { + getNetworkSettings.mockResolvedValue( + settings({ trusted_proxy_hops: 1, forwarded_chain: '198.51.100.7, 203.0.113.50' }) + ); + render(NetworkSettingsCard); + + expect(await screen.findByText(/arrived with 2 forwarded addresses/)).toBeTruthy(); + }); + + test('no hint when the setting already matches the chain length', async () => { + getNetworkSettings.mockResolvedValue( + settings({ trusted_proxy_hops: 1, forwarded_chain: '198.51.100.7' }) + ); + render(NetworkSettingsCard); + + await screen.findByText('198.51.100.7'); + expect(screen.queryByText(/arrived with/)).toBeNull(); + }); + + test('states the mis-set risk rather than only exposing a number', async () => { + getNetworkSettings.mockResolvedValue(settings()); + render(NetworkSettingsCard); + + expect(await screen.findByText(/Count your proxies/)).toBeTruthy(); + }); + + test('offers a retry when loading fails', async () => { + getNetworkSettings.mockRejectedValue(new Error('boom')); + render(NetworkSettingsCard); + + const retry = await screen.findByRole('button', { name: 'Try again' }); + getNetworkSettings.mockResolvedValue(settings()); + await fireEvent.click(retry); + await screen.findByText('198.51.100.7'); + }); +}); diff --git a/web/src/routes/admin/integrations/+page.svelte b/web/src/routes/admin/integrations/+page.svelte index 7915ec50..e69c25b1 100644 --- a/web/src/routes/admin/integrations/+page.svelte +++ b/web/src/routes/admin/integrations/+page.svelte @@ -27,6 +27,7 @@ import { errCode } from '$lib/api/errors'; import { pushToast } from '$lib/stores/toast.svelte'; import Modal from '$lib/components/Modal.svelte'; + import NetworkSettingsCard from '$lib/components/NetworkSettingsCard.svelte'; import type { LidarrConfig, LidarrTestResult } from '$lib/api/types'; // Lidarr connection panel. The "saved api key" is masked as "***" on GET — @@ -820,6 +821,12 @@ + + +