From ca50451f9cf4d48d0c7d58c37b80c52f64a5385b Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Fri, 4 Sep 2026 22:27:47 +0200 Subject: [PATCH] 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 | 5 +- pkg/service/soundtouchweb/websocket.go | 51 +++++++++++ .../soundtouchweb/websocket_origin_test.go | 84 ++++++++++++++++--- pkg/service/soundtouchweb/websocket_test.go | 52 ++++++++---- 4 files changed, 165 insertions(+), 27 deletions(-) diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index bb2940b1..e027b07b 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/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, + }, } } diff --git a/pkg/service/soundtouchweb/websocket.go b/pkg/service/soundtouchweb/websocket.go index 94dee3de..c5259e33 100644 --- a/pkg/service/soundtouchweb/websocket.go +++ b/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 diff --git a/pkg/service/soundtouchweb/websocket_origin_test.go b/pkg/service/soundtouchweb/websocket_origin_test.go index 88ee0bb7..57ade8c7 100644 --- a/pkg/service/soundtouchweb/websocket_origin_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/service/soundtouchweb/websocket_test.go b/pkg/service/soundtouchweb/websocket_test.go index 7b6b6ada..e2a42861 100644 --- a/pkg/service/soundtouchweb/websocket_test.go +++ b/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)