fix(player): ignore port when checking WebSocket handshake origin

Gorilla's default same-origin CheckOrigin compares Origin and Host as
raw strings, port included. This repo's own documented nginx reverse-
proxy config forwards a portless Host header (nginx's $host never
includes the port, unlike $http_host) regardless of what public port
the proxy listens on. That's harmless on the scheme's default port
(the browser's Origin also omits it there), but on a non-default
public port (e.g. :8443, a realistic multi-service-hosting shape) the
browser's Origin keeps the port while the forwarded Host doesn't --
gorilla's strict compare then 403s every WebSocket handshake, silently
breaking the player's live updates in a deployment topology the docs
actively recommend.

Add checkWebSocketOrigin/sameHostIgnoringPort: gorilla's own default
policy, but comparing hostname only. Same-origin and cross-hostname
behavior is unchanged; only a port mismatch on an otherwise-matching
hostname is now tolerated. Also extracted newTestWebSocketServer,
shared by dialTestWebSocket and the origin-policy test, instead of the
origin test re-implementing the same httptest scaffolding inline.

Found in code review of PR #669 (findings #1, #2).

pkg/service/soundtouchweb/handler.go

@@ -147,11 +147,12 @@ type DeviceEntry struct {
 
 // NewWebApp creates a new WebApp instance for SPA mode
 func NewWebApp() *WebApp {
-	// Leave Upgrader.CheckOrigin nil to use Gorilla's same-origin policy while
-	// retaining support for non-browser clients that omit the Origin header.
 	return &WebApp{
 		devices:   make(map[string]*webtypes.DeviceConnection),
 		WSClients: make(map[*websocket.Conn]*sync.Mutex),
+		Upgrader: websocket.Upgrader{
+			CheckOrigin: checkWebSocketOrigin,
+		},
 	}
 }
 

pkg/service/soundtouchweb/websocket.go

@@ -5,8 +5,11 @@ import (
 	"encoding/json"
 	"errors"
 	"log"
+	"net"
 	"net/http"
+	"net/url"
 	"reflect"
+	"strings"
 	"sync"
 	"time"
 
@@ -18,6 +21,54 @@ import (
 
 const defaultWebSocketWriteTimeout = 2 * time.Second
 
+// checkWebSocketOrigin is gorilla's own same-origin default (fail the
+// handshake only when an Origin header is present and doesn't match the
+// request host), except the comparison ignores port -- see
+// sameHostIgnoringPort for why.
+func checkWebSocketOrigin(r *http.Request) bool {
+	origin := r.Header.Get("Origin")
+	if origin == "" {
+		return true
+	}
+
+	u, err := url.Parse(origin)
+	if err != nil {
+		return false
+	}
+
+	return sameHostIgnoringPort(u.Host, r.Host)
+}
+
+// sameHostIgnoringPort reports whether a and b name the same hostname,
+// ignoring any port suffix.
+//
+// A reverse proxy commonly forwards a portless Host header regardless of
+// what public port it's listening on -- nginx's $host variable never
+// includes the port, unlike $http_host -- while a browser's Origin header
+// for a WebSocket handshake keeps an explicit, non-default port. Comparing
+// ports as well as host would reject that handshake whenever the proxy's
+// public listener uses a non-default port (e.g. :8443, a realistic shape
+// for multi-service hosting behind one proxy), even though the hostname
+// genuinely matches. This project's documented reverse-proxy config
+// (HTTPS-SETUP.md) relies on exactly that forwarding behavior.
+//
+// Trade-off: ignoring port also means two unrelated services sharing one
+// hostname on different ports would not be distinguished by this check
+// alone. Accepted here since gorilla's own default already doesn't check
+// scheme either, and the alternative is silently breaking the documented,
+// encouraged reverse-proxy deployment.
+func sameHostIgnoringPort(a, b string) bool {
+	if h, _, err := net.SplitHostPort(a); err == nil {
+		a = h
+	}
+
+	if h, _, err := net.SplitHostPort(b); err == nil {
+		b = h
+	}
+
+	return strings.EqualFold(a, b)
+}
+
 type webSocketWriter interface {
 	SetWriteDeadline(time.Time) error
 	WriteJSON(interface{}) error

pkg/service/soundtouchweb/websocket_origin_test.go

@@ -2,7 +2,7 @@ package soundtouchweb
 
 import (
 	"net/http"
-	"net/http/httptest"
+	"net/url"
 	"strings"
 	"testing"
 
@@ -11,17 +11,29 @@ import (
 
 func TestWebSocketOriginPolicy(t *testing.T) {
 	app := NewWebApp()
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		conn, err := app.Upgrader.Upgrade(w, r, nil)
-		if err != nil {
-			return
-		}
-
-		_ = conn.Close()
-	}))
-	defer server.Close()
+	server, _, release := newTestWebSocketServer(t, app)
+	defer func() {
+		release()
+		server.Close()
+	}()
 
 	webSocketURL := "ws" + strings.TrimPrefix(server.URL, "http")
+
+	// A same-hostname origin with a different port, simulating a reverse
+	// proxy whose forwarded Host header drops the public port (nginx's
+	// $host) while the browser's Origin keeps it -- see
+	// sameHostIgnoringPort's doc comment.
+	serverURL, err := url.Parse(server.URL)
+	if err != nil {
+		t.Fatalf("parse test server URL: %v", err)
+	}
+
+	if serverURL.Port() == "18443" {
+		t.Fatal("test server happened to bind the port this test uses as a deliberately different one")
+	}
+
+	mismatchedPortOrigin := serverURL.Scheme + "://" + serverURL.Hostname() + ":18443"
+
 	tests := []struct {
 		name       string
 		origin     string
@@ -29,6 +41,7 @@ func TestWebSocketOriginPolicy(t *testing.T) {
 	}{
 		{name: "originless non-browser client", wantStatus: http.StatusSwitchingProtocols},
 		{name: "same origin", origin: server.URL, wantStatus: http.StatusSwitchingProtocols},
+		{name: "same host, mismatched port (reverse proxy)", origin: mismatchedPortOrigin, wantStatus: http.StatusSwitchingProtocols},
 		{name: "cross origin", origin: "https://attacker.example", wantStatus: http.StatusForbidden},
 	}
 
@@ -73,3 +86,54 @@ func TestWebSocketOriginPolicy(t *testing.T) {
 		})
 	}
 }
+
+func TestSameHostIgnoringPort(t *testing.T) {
+	tests := []struct {
+		name string
+		a    string
+		b    string
+		want bool
+	}{
+		{name: "identical host and port", a: "example.com:8443", b: "example.com:8443", want: true},
+		{name: "same host, mismatched port", a: "example.com:8443", b: "example.com", want: true},
+		{name: "same host, no ports either side", a: "example.com", b: "example.com", want: true},
+		{name: "different host, same port", a: "example.com:8443", b: "attacker.example:8443", want: false},
+		{name: "different host, no ports", a: "example.com", b: "attacker.example", want: false},
+		{name: "case-insensitive host", a: "Example.com", b: "example.com", want: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got := sameHostIgnoringPort(test.a, test.b); got != test.want {
+				t.Errorf("sameHostIgnoringPort(%q, %q) = %v, want %v", test.a, test.b, got, test.want)
+			}
+		})
+	}
+}
+
+func TestCheckWebSocketOrigin(t *testing.T) {
+	tests := []struct {
+		name   string
+		origin string
+		host   string
+		want   bool
+	}{
+		{name: "no origin header (non-browser client)", host: "example.com:8443", want: true},
+		{name: "same host, mismatched port (reverse proxy)", origin: "https://example.com:8443", host: "example.com", want: true},
+		{name: "different host", origin: "https://attacker.example", host: "example.com:8443", want: false},
+		{name: "malformed origin", origin: "://not a url", host: "example.com", want: false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			r := &http.Request{Host: test.host, Header: http.Header{}}
+			if test.origin != "" {
+				r.Header.Set("Origin", test.origin)
+			}
+
+			if got := checkWebSocketOrigin(r); got != test.want {
+				t.Errorf("checkWebSocketOrigin(origin=%q, host=%q) = %v, want %v", test.origin, test.host, got, test.want)
+			}
+		})
+	}
+}

pkg/service/soundtouchweb/websocket_test.go

@@ -340,19 +340,7 @@ func (writer *deadlineBlockingWebSocketWriter) WriteMessage(int, []byte) error {
 func dialTestWebSocket(t *testing.T, app *WebApp) (remote *websocket.Conn, client *websocket.Conn, cleanup func()) {
 	t.Helper()
 
-	serverConnection := make(chan *websocket.Conn, 1)
-	releaseServer := make(chan struct{})
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		conn, err := app.Upgrader.Upgrade(w, r, nil)
-		if err != nil {
-			t.Errorf("upgrade test WebSocket: %v", err)
-
-			return
-		}
-		serverConnection <- conn
-		<-releaseServer
-		_ = conn.Close()
-	}))
+	server, serverConnection, release := newTestWebSocketServer(t, app)
 
 	client, response, err := websocket.DefaultDialer.Dial(
 		"ws"+strings.TrimPrefix(server.URL, "http"), nil,
@@ -361,7 +349,7 @@ func dialTestWebSocket(t *testing.T, app *WebApp) (remote *websocket.Conn, clien
 		_ = response.Body.Close()
 	}
 	if err != nil {
-		close(releaseServer)
+		release()
 		server.Close()
 		t.Fatalf("dial test WebSocket: %v", err)
 	}
@@ -369,12 +357,46 @@ func dialTestWebSocket(t *testing.T, app *WebApp) (remote *websocket.Conn, clien
 	remote = <-serverConnection
 
 	return remote, client, func() {
-		close(releaseServer)
+		release()
 		_ = client.Close()
 		server.Close()
 	}
 }
 
+// newTestWebSocketServer starts an httptest.Server that upgrades every
+// request through app.Upgrader and holds each connection open until
+// release is called. Shared by dialTestWebSocket (which always expects a
+// successful handshake) and tests that also need to exercise an *expected*
+// rejection (e.g. origin-policy tests), which is why the handshake failure
+// path here stays silent rather than failing the test.
+func newTestWebSocketServer(t *testing.T, app *WebApp) (
+	server *httptest.Server,
+	serverConnection <-chan *websocket.Conn,
+	release func(),
+) {
+	t.Helper()
+
+	// Buffered generously: some callers (e.g. table-driven origin-policy
+	// tests) dial the same server multiple times without draining
+	// serverConnection, and an unread successful upgrade must not block
+	// its own handler goroutine on this channel send.
+	conns := make(chan *websocket.Conn, 8)
+	releaseServer := make(chan struct{})
+	var releaseOnce sync.Once
+
+	server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		conn, err := app.Upgrader.Upgrade(w, r, nil)
+		if err != nil {
+			return
+		}
+		conns <- conn
+		<-releaseServer
+		_ = conn.Close()
+	}))
+
+	return server, conns, func() { releaseOnce.Do(func() { close(releaseServer) }) }
+}
+
 func TestConnWriteSerializesWritesToSameConnection(t *testing.T) {
 	app := NewWebApp()
 	remote, _, cleanup := dialTestWebSocket(t, app)