From 28d7675fc4d0d534464a01ea407cd019be22ef43 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 28 Jun 2026 12:59:08 +0200 Subject: [PATCH] refactor(handlers): resolve client IP via a clientHost helper Route every HTTP read of the client IP through a single clientHost(r) helper backed by chi's new middleware.GetClientIP, falling back to the socket peer from r.RemoteAddr. AddDeviceToAccount now takes a bare client host instead of a "host:port" RemoteAddr. Behavior is unchanged in this commit (no ClientIP middleware is wired yet, so the fallback is always taken); a follow-up wires middleware.ClientIP and removes the deprecated middleware.RealIP. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/service/handlers/auth_probe.go | 12 ++++++++++ pkg/service/handlers/deprecated_routes.go | 2 +- pkg/service/handlers/handlers_marge.go | 18 ++++++-------- pkg/service/handlers/handlers_tts.go | 2 +- pkg/service/handlers/handlers_unsupported.go | 2 +- .../handlers/middleware_peer_observer.go | 4 +--- pkg/service/marge/marge.go | 24 +++++++++---------- 7 files changed, 34 insertions(+), 30 deletions(-) diff --git a/pkg/service/handlers/auth_probe.go b/pkg/service/handlers/auth_probe.go index b57ac6b..4f82b78 100644 --- a/pkg/service/handlers/auth_probe.go +++ b/pkg/service/handlers/auth_probe.go @@ -11,6 +11,7 @@ import ( "time" "github.com/gesellix/bose-soundtouch/pkg/client" + "github.com/go-chi/chi/v5/middleware" ) const ( @@ -147,6 +148,17 @@ func clientHostFromRemoteAddr(remoteAddr string) string { return remoteAddr } +// clientHost returns the resolved client IP for r: chi's middleware.GetClientIP +// (populated by the ClientIP middleware) when set, falling back to the socket +// peer host from r.RemoteAddr. Returns a bare IP (no port). +func clientHost(r *http.Request) string { + if ip := middleware.GetClientIP(r.Context()); ip != "" { + return ip + } + + return clientHostFromRemoteAddr(r.RemoteAddr) +} + // dnsProbeSpeakerRequest is the JSON body for POST /setup/health/dns-path-probe. type dnsProbeSpeakerRequest struct { DeviceID string `json:"deviceId,omitempty"` diff --git a/pkg/service/handlers/deprecated_routes.go b/pkg/service/handlers/deprecated_routes.go index 1e33b31..22b0b7f 100644 --- a/pkg/service/handlers/deprecated_routes.go +++ b/pkg/service/handlers/deprecated_routes.go @@ -78,7 +78,7 @@ func (s *Server) DeprecatedRouteMiddleware(next http.Handler) http.Handler { if s.deprecatedRoutes.record(key) { log.Printf("[deprecated-route] %s used by client=%s — use /api%s instead; "+ "the legacy path still works but is slated for removal in a future major release", - sanitizeLog(key), sanitizeLog(clientHostFromRemoteAddr(r.RemoteAddr)), sanitizeLog(pattern)) + sanitizeLog(key), sanitizeLog(clientHost(r)), sanitizeLog(pattern)) } }) } diff --git a/pkg/service/handlers/handlers_marge.go b/pkg/service/handlers/handlers_marge.go index c28f5a8..fef2bd8 100644 --- a/pkg/service/handlers/handlers_marge.go +++ b/pkg/service/handlers/handlers_marge.go @@ -8,7 +8,6 @@ import ( "io" "log" "math/big" - "net" "net/http" "strconv" "time" @@ -244,7 +243,7 @@ func (s *Server) HandleMargePowerOn(w http.ResponseWriter, r *http.Request) { log.Printf("[Marge] Failed to parse power_on body: %v", err) // Fallback to remote address if body parsing fails - if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + if host := clientHost(r); host != "" { go s.PrimeDeviceWithSpotify(host) } @@ -292,14 +291,11 @@ func (s *Server) HandleMargePowerOn(w http.ResponseWriter, r *http.Request) { // Prefer the TCP source address over the body's self-reported IP for // any outbound credential push. The body field is attacker-controllable // (a malicious LAN-resident speaker can set it to any value), while - // r.RemoteAddr is the actual peer — and if the service runs behind a - // trusted reverse proxy, the TrustedRealIP middleware has already - // rewritten it from X-Real-IP / X-Forwarded-For. We log when the two + // clientHost(r) is the actual peer — and if the service runs behind a + // trusted reverse proxy, the ClientIP middleware has already populated + // the context from X-Forwarded-For. We log when the two // disagree so the discrepancy is investigable but never trust the body. - remoteHost := "" - if h, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { - remoteHost = h - } + remoteHost := clientHost(r) if deviceIP != "" && remoteHost != "" && deviceIP != remoteHost { log.Printf("[Marge] power_on body IP %q differs from TCP source %q for device %s — using TCP source for credential push", @@ -588,7 +584,7 @@ func (s *Server) HandleMargeAddDevice(w http.ResponseWriter, r *http.Request) { return } - deviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) + deviceID, data, err := marge.AddDeviceToAccount(s.ds, account, body, clientHost(r)) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -657,7 +653,7 @@ func (s *Server) HandleMargeUpdateDevice(w http.ResponseWriter, r *http.Request) return } - _, data, err := marge.AddDeviceToAccount(s.ds, account, body, r.RemoteAddr) + _, data, err := marge.AddDeviceToAccount(s.ds, account, body, clientHost(r)) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/pkg/service/handlers/handlers_tts.go b/pkg/service/handlers/handlers_tts.go index 4c56d61..9e93cd7 100644 --- a/pkg/service/handlers/handlers_tts.go +++ b/pkg/service/handlers/handlers_tts.go @@ -151,7 +151,7 @@ func buildCustomPlaybackURL(base, audioURL, name string) string { func (s *Server) HandleSpeakerAuth(w http.ResponseWriter, r *http.Request) { token := r.Header.Get("Apikeyheader") if token != "" && s.authProbes != nil { - if s.authProbes.observe(token, clientHostFromRemoteAddr(r.RemoteAddr)) { + if s.authProbes.observe(token, clientHost(r)) { // Active DNS-path probe: the callback arrival already proved the // speaker resolved a Bose host through AfterTouch. Returning 403 // makes the speaker treat the key as invalid and refuse the diff --git a/pkg/service/handlers/handlers_unsupported.go b/pkg/service/handlers/handlers_unsupported.go index c7091e2..e3db217 100644 --- a/pkg/service/handlers/handlers_unsupported.go +++ b/pkg/service/handlers/handlers_unsupported.go @@ -25,7 +25,7 @@ const maxUnsupportedBodyLog = 2048 // relies on the route we find out and restore it instead of silently breaking // it during the refactor. func (s *Server) HandleUnsupported(w http.ResponseWriter, r *http.Request) { - client := clientHostFromRemoteAddr(r.RemoteAddr) + client := clientHost(r) var body []byte if r.Body != nil { diff --git a/pkg/service/handlers/middleware_peer_observer.go b/pkg/service/handlers/middleware_peer_observer.go index a4d8dc7..14f9083 100644 --- a/pkg/service/handlers/middleware_peer_observer.go +++ b/pkg/service/handlers/middleware_peer_observer.go @@ -1,7 +1,6 @@ package handlers import ( - "net" "net/http" "time" @@ -21,8 +20,7 @@ import ( // request. func (s *Server) PeerObserverMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err == nil && host != "" { + if host := clientHost(r); host != "" { s.peerObserver.Signal(host, setup.PeerHit{Path: r.URL.Path, At: time.Now()}) } diff --git a/pkg/service/marge/marge.go b/pkg/service/marge/marge.go index c96b01d..dce8b29 100644 --- a/pkg/service/marge/marge.go +++ b/pkg/service/marge/marge.go @@ -2224,11 +2224,11 @@ func formatRecentResponse(recentObj *models.ServiceRecent, matchingSrc *models.C // handlers — the persistence layer doesn't distinguish; only the // response status differs. // -// remoteAddr is the speaker's address as seen by the HTTP server -// (r.RemoteAddr, "host:port"). When the request body doesn't carry -// an `` and the datastore has no IP for this device yet, -// we fall back to remoteAddr's host portion. An empty remoteAddr -// is treated as "no fallback available" — never errors. +// clientHost is the speaker's resolved client IP (bare IP, no port), +// as returned by handlers.clientHost(r). When the request body doesn't +// carry an `` and the datastore has no IP for this device +// yet, we fall back to clientHost. An empty or non-IP clientHost is +// treated as "no fallback available" — never errors. // // Timestamps: // - CreatedOn is preserved from any existing datastore record so a @@ -2238,7 +2238,7 @@ func formatRecentResponse(recentObj *models.ServiceRecent, matchingSrc *models.C // // Returns the persisted deviceID and the marge XML response shape // (``). -func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byte, remoteAddr string) (string, []byte, error) { +func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byte, clientHost string) (string, []byte, error) { var newDeviceElem struct { DeviceID string `xml:"deviceid,attr"` Name string `xml:"name"` @@ -2277,14 +2277,12 @@ func AddDeviceToAccount(ds *datastore.DataStore, account string, sourceXML []byt // hitting us through a different network path right now, e.g. // SSH port-forward, and the persisted IP is the one other // flows like DNS hints care about). Fall back to the inbound - // connection's remote address only when there's no existing - // IP to preserve. Invalid remoteAddr leaves info.IPAddress - // empty, which the merge then handles. + // connection's client host only when there's no existing + // IP to preserve. An empty or non-IP clientHost leaves + // info.IPAddress empty, which the merge then handles. if existing == nil || existing.IPAddress == "" { - if remoteAddr != "" { - if host, _, splitErr := net.SplitHostPort(remoteAddr); splitErr == nil { - info.IPAddress = host - } + if clientHost != "" && net.ParseIP(clientHost) != nil { + info.IPAddress = clientHost } }