From 4f561944a3dd74c5981db73a82c5e050a80dca21 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 5 Jun 2026 12:56:02 +0200 Subject: [PATCH] fix(bmx): strip trailing slash from server_url so TuneIn playback routes A server_url configured with a trailing slash (e.g. http://host:8000/) flowed verbatim into the BMX registry base ("{BMX_SERVER}/bmx/tunein"), so speakers were handed "http://host:8000//bmx/tunein" and requested "//bmx/tunein/v1/playback/station/{id}". The chi router does not match the doubled-slash path, so TuneIn playback returned 404 and the speaker reported INVALID_SOURCE. Confirmed from a reporter's diagnostic export. - Add NormalizeServerURL (trim whitespace + trailing slashes); apply in NewServer so the BMX base is always clean. - Normalize server_url at ingestion in main (flag + persisted) so the margeServerUrl/bmxRegistryUrl pushed to speakers stays clean too. - Normalize in the live settings-update path so a UI-saved trailing slash is trimmed before validate/persist. - Mount chi middleware.CleanPath as a defensive net: any "//" path collapses to "/" before routing, regardless of source. - Regression tests: NormalizeServerURL table + BMX registry must not emit "//bmx"/"//media" for a trailing-slash server_url. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/soundtouch-service/main.go | 12 +++- pkg/service/handlers/bmx_registry_test.go | 67 +++++++++++++++++++++++ pkg/service/handlers/handlers_setup.go | 5 ++ pkg/service/handlers/server.go | 13 ++++- 4 files changed, 95 insertions(+), 2 deletions(-) diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 6aec9f4..7fe66e1 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -693,6 +693,9 @@ func loadConfig(c *cli.Context) serviceConfig { if serverURL == "" { serverURL = "http://" + hostname + ":" + port } + // Strip a trailing slash so it cannot leak into the BMX registry base or the + // margeServerUrl/bmxRegistryUrl pushed to speakers during migration. + serverURL = handlers.NormalizeServerURL(serverURL) httpsPort := c.String("https-port") @@ -878,7 +881,7 @@ func applyPersistedSettings(ds *datastore.DataStore, config *serviceConfig) data } if persisted.ServerURL != "" { - config.serverURL = persisted.ServerURL + config.serverURL = handlers.NormalizeServerURL(persisted.ServerURL) } if persisted.HTTPServerURL != "" { @@ -1060,6 +1063,13 @@ func startDeviceDiscovery(server *handlers.Server) { func setupRouter(server *handlers.Server, stockholmHandler *stockholm.Handler) *chi.Mux { r := chi.NewRouter() + // CleanPath collapses duplicate slashes ("//bmx/..." -> "/bmx/...") and + // resolves . / .. before routing. Defensive net for the double-slash + // playback bug: even if a misconfigured base URL hands a speaker a "//bmx" + // path, it still reaches the right handler instead of 404ing. Runs first so + // every downstream middleware and the recorder see the cleaned path. + r.Use(middleware.CleanPath) + // TrustedRealIP must run before any handler that reads r.RemoteAddr — // SnapshotMiddleware captures the request, and several handlers // (HandleMargePowerOn, etc.) inspect the source IP. The middleware is diff --git a/pkg/service/handlers/bmx_registry_test.go b/pkg/service/handlers/bmx_registry_test.go index 14a6357..1729f75 100644 --- a/pkg/service/handlers/bmx_registry_test.go +++ b/pkg/service/handlers/bmx_registry_test.go @@ -105,3 +105,70 @@ func TestHandleBMXRegistry_DNSDependent(t *testing.T) { } }) } + +func TestNormalizeServerURL(t *testing.T) { + cases := []struct { + in, want string + }{ + {"http://host:8000", "http://host:8000"}, + {"http://host:8000/", "http://host:8000"}, + {"http://host:8000///", "http://host:8000"}, + {" http://host:8000/ ", "http://host:8000"}, + {"https://127.0.0.1", "https://127.0.0.1"}, + {"", ""}, + } + for _, c := range cases { + if got := NormalizeServerURL(c.in); got != c.want { + t.Errorf("NormalizeServerURL(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// TestHandleBMXRegistry_TrailingSlashServerURL is a regression test for the +// trailing-slash double-slash bug: a server_url configured with a trailing +// slash must not produce a "//bmx/..." +// base URL. The speaker concatenates "/v1/playback/station/{id}" onto the base, +// so a doubled slash yields a "//bmx/tunein/..." request the router 404s and +// TuneIn playback fails with INVALID_SOURCE. +func TestHandleBMXRegistry_TrailingSlashServerURL(t *testing.T) { + tempDir, err := os.MkdirTemp("", "bmx-registry-slash-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + // Operator typed a trailing slash (the reported trailing-slash case). + server := NewServer(ds, nil, "https://127.0.0.1/", false, false, false) + server.SetDNSSettings(false, "", "") + + req := httptest.NewRequest("GET", "/bmx/v1/services", nil) + w := httptest.NewRecorder() + server.HandleBMXRegistry(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("Expected status 200, got %d", w.Code) + } + + body := w.Body.String() + if strings.Contains(body, "//bmx") || strings.Contains(body, "//media") { + t.Errorf("registry response contains a doubled slash from the trailing-slash server_url:\n%s", body) + } + + var resp map[string]interface{} + if err := json.Unmarshal([]byte(body), &resp); err != nil { + t.Fatalf("Failed to unmarshal response: %v", err) + } + for _, s := range resp["bmx_services"].([]interface{}) { + service := s.(map[string]interface{}) + if service["id"].(map[string]interface{})["name"] == "TUNEIN" { + if baseURL := service["baseUrl"].(string); baseURL != "https://127.0.0.1/bmx/tunein" { + t.Errorf("Expected baseUrl https://127.0.0.1/bmx/tunein, got %s", baseURL) + } + return + } + } + t.Error("TuneIn service not found in registry") +} diff --git a/pkg/service/handlers/handlers_setup.go b/pkg/service/handlers/handlers_setup.go index de79e04..96782e7 100644 --- a/pkg/service/handlers/handlers_setup.go +++ b/pkg/service/handlers/handlers_setup.go @@ -294,6 +294,11 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { log.Printf("[DNS] DNS Discovery enabled without explicit upstreams, will try system DNS.") } + // Strip a trailing slash before validating/persisting: it would otherwise + // flow into the BMX registry base and produce "//bmx/..." playback requests + // the router 404s. + settings.ServerURL = NormalizeServerURL(settings.ServerURL) + // Validate server_url: the same value the DNS server uses to derive its // intercept IP. Reject anything that does not resolve to a routable IP so // users see the error in the UI instead of getting a silently-broken setup diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 26c0b7d..58e1d9d 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -108,12 +108,23 @@ var bufferPool = sync.Pool{ }, } +// NormalizeServerURL trims surrounding whitespace and any trailing slashes from +// a configured server URL. A trailing slash poisons every URL built by string +// concatenation from it, most visibly the BMX registry base ("{BMX_SERVER}/bmx/ +// tunein" in bmx_services.json): it would otherwise hand a speaker +// "http://host:8000//bmx/tunein" and make it request "//bmx/tunein/...", a path +// the chi router does not match, so playback 404s. It also keeps {MEDIA_SERVER} +// and the OAuth redirect URIs free of a stray double slash. +func NormalizeServerURL(serverURL string) string { + return strings.TrimRight(strings.TrimSpace(serverURL), "/") +} + // NewServer creates a new SoundTouch service server. func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, redactLogs, logBodies, recordEnabled bool) *Server { s := &Server{ ds: ds, sm: sm, - serverURL: serverURL, + serverURL: NormalizeServerURL(serverURL), redactLogs: redactLogs, logBodies: logBodies, recordEnabled: recordEnabled,