From 3746a30126513b27b5b3bef80fd448412b5d0ab3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Wed, 15 Jul 2026 01:13:46 +0100 Subject: [PATCH] fix: enforce origin validation on websocket upgrades (#1698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: enforce origin validation on websocket upgrades The gateway /ws upgrader hardcoded CheckOrigin to true, so any web page could open a socket to the local backend and drive session/shell actions (cross-site WebSocket hijacking → RCE on the host). Both upgraders now share checkWebSocketOrigin: no Origin header is allowed (non-browser clients), same-hostname and loopback↔loopback origins are allowed, everything else is rejected. Replaces the terminal handler's prefix-based check, which accepted origins like http://localhost.attacker.tld. Co-Authored-By: Claude Opus 4.7 * refactor: share origin trust policy between cors and websocket checks Review feedback on #1698: the CORS middleware and the WebSocket upgrader carried two copies of the same origin policy, which could silently diverge. Both now delegate to httpmw.AllowedOrigin, which also rejects URL components that never appear in a browser Origin header (userinfo, path, query, fragment, opaque data). Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- .../backend/internal/backendapp/middleware.go | 42 +------ apps/backend/internal/common/httpmw/origin.go | 70 +++++++++++ .../internal/common/httpmw/origin_test.go | 66 ++++++++++ .../internal/gateway/websocket/handler.go | 6 +- .../gateway/websocket/handler_origin_test.go | 117 ++++++++++++++++++ .../internal/gateway/websocket/origin.go | 23 ++++ .../internal/gateway/websocket/origin_test.go | 40 ++++++ .../gateway/websocket/terminal_handler.go | 43 ------- .../websocket/terminal_handler_test.go | 56 --------- 9 files changed, 320 insertions(+), 143 deletions(-) create mode 100644 apps/backend/internal/common/httpmw/origin.go create mode 100644 apps/backend/internal/common/httpmw/origin_test.go create mode 100644 apps/backend/internal/gateway/websocket/handler_origin_test.go create mode 100644 apps/backend/internal/gateway/websocket/origin.go create mode 100644 apps/backend/internal/gateway/websocket/origin_test.go diff --git a/apps/backend/internal/backendapp/middleware.go b/apps/backend/internal/backendapp/middleware.go index 2f5d860a34..1f8702c7f1 100644 --- a/apps/backend/internal/backendapp/middleware.go +++ b/apps/backend/internal/backendapp/middleware.go @@ -1,19 +1,18 @@ package backendapp import ( - "net" "net/http" - "net/url" - "strings" "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/common/httpmw" ) // corsMiddleware returns a CORS middleware for HTTP and WebSocket connections. func corsMiddleware() gin.HandlerFunc { return func(c *gin.Context) { if origin := c.Request.Header.Get("Origin"); origin != "" { - if !isAllowedCORSOrigin(origin, c.Request.Host) { + if !httpmw.AllowedOrigin(origin, c.Request.Host) { if c.Request.Method == http.MethodOptions { c.AbortWithStatus(http.StatusForbidden) return @@ -40,38 +39,3 @@ func corsMiddleware() gin.HandlerFunc { c.Next() } } - -func isAllowedCORSOrigin(origin string, requestHost string) bool { - parsed, err := url.Parse(origin) - if err != nil || parsed.Scheme == "" || parsed.Host == "" { - return false - } - if parsed.Scheme != "http" && parsed.Scheme != "https" { - return false - } - - originHost := normalizeCORSHost(parsed.Hostname()) - host := normalizeCORSHost(requestHost) - if originHost == "" || host == "" { - return false - } - - return originHost == host || (isLoopbackCORSHost(originHost) && isLoopbackCORSHost(host)) -} - -func normalizeCORSHost(host string) string { - if parsedHost, _, err := net.SplitHostPort(host); err == nil { - host = parsedHost - } - - return strings.ToLower(strings.Trim(host, "[]")) -} - -func isLoopbackCORSHost(host string) bool { - if host == "localhost" { - return true - } - - ip := net.ParseIP(host) - return ip != nil && ip.IsLoopback() -} diff --git a/apps/backend/internal/common/httpmw/origin.go b/apps/backend/internal/common/httpmw/origin.go new file mode 100644 index 0000000000..663e0a8908 --- /dev/null +++ b/apps/backend/internal/common/httpmw/origin.go @@ -0,0 +1,70 @@ +package httpmw + +import ( + "net" + "net/url" + "strings" +) + +// AllowedOrigin reports whether a browser Origin header value is trusted to +// reach this backend, given the Host of the incoming request. It is the single +// origin trust policy shared by the HTTP CORS middleware and the WebSocket +// upgraders so the two cannot silently diverge. +// +// Policy: +// - Origin hostname equals the request hostname: allow. Ports are +// deliberately not compared — the dev SPA connects cross-port over the +// same hostname (apps/web/lib/config.ts), and content served from the +// backend's own hostname implies a process on that host, which could +// open a direct connection without an Origin header anyway. +// - Origin and request host are both loopback: allow (dev servers and the +// desktop shell talk to the backend across different loopback ports). +// - Anything else: deny. +// +// Hostnames are compared exactly after parsing — a prefix check against +// "http://localhost" would also accept http://localhost.attacker.tld. +// +// The empty-Origin case is intentionally not handled here; callers decide +// (CORS answers with a wildcard, the WebSocket upgrader allows non-browser +// clients such as the CLI or curl). +func AllowedOrigin(origin, requestHost string) bool { + parsed, err := url.Parse(origin) + if err != nil || !isWellFormedOrigin(parsed) { + return false + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return false + } + + originHost := normalizeHost(parsed.Hostname()) + host := normalizeHost(requestHost) + if originHost == "" || host == "" { + return false + } + + return originHost == host || (isLoopbackHost(originHost) && isLoopbackHost(host)) +} + +// isWellFormedOrigin rejects URL components that never appear in a +// browser-sent Origin header (userinfo, path, query, fragment, opaque data); +// url.Parse accepts them, but the strict policy treats them as malformed. +func isWellFormedOrigin(u *url.URL) bool { + return u.Scheme != "" && u.Host != "" && + u.User == nil && u.Opaque == "" && u.Path == "" && + u.RawQuery == "" && !u.ForceQuery && u.Fragment == "" +} + +func normalizeHost(host string) string { + if parsedHost, _, err := net.SplitHostPort(host); err == nil { + host = parsedHost + } + return strings.ToLower(strings.Trim(host, "[]")) +} + +func isLoopbackHost(host string) bool { + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} diff --git a/apps/backend/internal/common/httpmw/origin_test.go b/apps/backend/internal/common/httpmw/origin_test.go new file mode 100644 index 0000000000..33db438fde --- /dev/null +++ b/apps/backend/internal/common/httpmw/origin_test.go @@ -0,0 +1,66 @@ +package httpmw + +import "testing" + +func TestAllowedOrigin(t *testing.T) { + tests := []struct { + name string + origin string + host string + want bool + }{ + // Loopback origin ↔ loopback host (dev servers, desktop shell) + {"localhost", "http://localhost", "localhost", true}, + {"localhost different ports", "http://localhost:3000", "localhost:8080", true}, + {"https localhost", "https://localhost", "localhost", true}, + {"127.0.0.1", "http://127.0.0.1", "127.0.0.1", true}, + {"127.0.0.1 different ports", "http://127.0.0.1:3000", "127.0.0.1:8080", true}, + {"localhost origin to 127.0.0.1 host", "http://localhost:3000", "127.0.0.1:8080", true}, + {"ipv6 loopback origin to ipv4 loopback host", "http://[::1]:3000", "127.0.0.1:8080", true}, + {"ipv6 loopback host", "http://localhost:3000", "[::1]:8080", true}, + + // Same hostname (ports ignored — see AllowedOrigin doc) + {"same origin", "https://example.com", "example.com", true}, + {"same origin with port", "https://example.com:443", "example.com:8080", true}, + {"same origin case insensitive", "https://Example.COM", "example.com", true}, + {"uppercase scheme", "HTTPS://example.com", "example.com", true}, + + // Cross-origin — reject + {"cross origin", "https://evil.com", "example.com", false}, + {"cross origin similar", "https://notexample.com", "example.com", false}, + {"ipv6 loopback origin to public host", "http://[::1]:3000", "example.com:8080", false}, + {"loopback origin to public host", "http://localhost:3000", "example.com", false}, + + // Loopback prefix-match bypass attempts — hostname must match exactly + {"localhost subdomain bypass", "http://localhost.attacker.tld", "localhost:8080", false}, + {"127.0.0.1 subdomain bypass", "http://127.0.0.1.attacker.tld:80", "127.0.0.1:8080", false}, + + // Malformed / non-http origins + {"empty origin", "", "example.com", false}, + {"malformed origin", "not-a-url", "example.com", false}, + {"null origin", "null", "localhost:8080", false}, + {"file scheme", "file:///etc/passwd", "localhost:8080", false}, + {"ws scheme", "ws://example.com", "example.com", false}, + + // Components that never appear in a browser Origin header — reject + {"userinfo", "https://user@example.com", "example.com", false}, + {"userinfo with password", "https://user:pass@example.com", "example.com", false}, + {"path", "https://example.com/path", "example.com", false}, + {"trailing slash", "https://example.com/", "example.com", false}, + {"query", "https://example.com?x=1", "example.com", false}, + {"empty query", "https://example.com?", "example.com", false}, + {"fragment", "https://example.com#frag", "example.com", false}, + {"opaque", "https:example.com", "example.com", false}, + + // Empty host — no match possible + {"empty request host", "https://example.com", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := AllowedOrigin(tt.origin, tt.host); got != tt.want { + t.Errorf("AllowedOrigin(%q, %q) = %v, want %v", tt.origin, tt.host, got, tt.want) + } + }) + } +} diff --git a/apps/backend/internal/gateway/websocket/handler.go b/apps/backend/internal/gateway/websocket/handler.go index 37147882e0..11643835fb 100644 --- a/apps/backend/internal/gateway/websocket/handler.go +++ b/apps/backend/internal/gateway/websocket/handler.go @@ -2,7 +2,6 @@ package websocket import ( "context" - "net/http" "github.com/gin-gonic/gin" "github.com/google/uuid" @@ -16,10 +15,7 @@ import ( var upgrader = gorillaws.Upgrader{ ReadBufferSize: 1024, WriteBufferSize: 1024, - CheckOrigin: func(r *http.Request) bool { - // TODO: In production, validate origin - return true - }, + CheckOrigin: checkWebSocketOrigin, } // Handler handles WebSocket connections diff --git a/apps/backend/internal/gateway/websocket/handler_origin_test.go b/apps/backend/internal/gateway/websocket/handler_origin_test.go new file mode 100644 index 0000000000..c5b63e28c2 --- /dev/null +++ b/apps/backend/internal/gateway/websocket/handler_origin_test.go @@ -0,0 +1,117 @@ +package websocket + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + gorillaws "github.com/gorilla/websocket" +) + +// startWSGateway serves the real /ws route backed by a running hub and returns +// the gateway plus the ws:// URL to dial. +func startWSGateway(t *testing.T) (*Gateway, string) { + t.Helper() + + g := NewGateway(newTestProxyLogger()) + + ctx, cancel := context.WithCancel(context.Background()) + hubDone := make(chan struct{}) + go func() { + defer close(hubDone) + g.Hub.Run(ctx) + }() + + gin.SetMode(gin.TestMode) + router := gin.New() + g.SetupRoutes(router) + srv := httptest.NewServer(router) + + t.Cleanup(func() { + srv.Close() + cancel() + <-hubDone + }) + + return g, "ws" + strings.TrimPrefix(srv.URL, "http") + "/ws" +} + +func dialWS(t *testing.T, wsURL, origin string) (*gorillaws.Conn, *http.Response, error) { + t.Helper() + + header := http.Header{} + if origin != "" { + header.Set("Origin", origin) + } + conn, resp, err := gorillaws.DefaultDialer.Dial(wsURL, header) + if resp != nil && resp.Body != nil { + t.Cleanup(func() { _ = resp.Body.Close() }) + } + return conn, resp, err +} + +// waitForNoClients blocks until every connection has unregistered from the hub +// so goleak sees clean read/write pumps at package teardown. +func waitForNoClients(t *testing.T, g *Gateway) { + t.Helper() + + deadline := time.Now().Add(2 * time.Second) + for g.Hub.GetClientCount() != 0 { + if time.Now().After(deadline) { + t.Fatalf("hub still has %d client(s)", g.Hub.GetClientCount()) + } + time.Sleep(5 * time.Millisecond) + } +} + +// TestHandleConnection_RejectsForeignOrigin is the CSWSH regression test: a +// browser page on an attacker-controlled origin must not be able to complete +// a WebSocket upgrade against the gateway. +func TestHandleConnection_RejectsForeignOrigin(t *testing.T) { + _, wsURL := startWSGateway(t) + + conn, resp, err := dialWS(t, wsURL, "https://attacker.example") + if err == nil { + _ = conn.Close() + t.Fatal("expected upgrade with foreign Origin to fail") + } + if resp == nil { + t.Fatalf("expected HTTP response on rejected upgrade, got err=%v", err) + } + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("rejected upgrade status = %d, want %d", resp.StatusCode, http.StatusForbidden) + } +} + +// TestHandleConnection_AllowsTrustedOrigins verifies legitimate clients still +// connect: same-origin browsers, loopback dev servers on another port, and +// non-browser clients that send no Origin header. +func TestHandleConnection_AllowsTrustedOrigins(t *testing.T) { + g, wsURL := startWSGateway(t) + + sameOrigin := "http" + strings.TrimSuffix(strings.TrimPrefix(wsURL, "ws"), "/ws") + + for name, origin := range map[string]string{ + "no origin": "", + "same origin": sameOrigin, + "loopback cross port": "http://localhost:5173", + "loopback 127 with port": "http://127.0.0.1:12345", + } { + t.Run(name, func(t *testing.T) { + conn, resp, err := dialWS(t, wsURL, origin) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("upgrade with origin %q failed (status %d): %v", origin, status, err) + } + _ = conn.Close() + waitForNoClients(t, g) + }) + } +} diff --git a/apps/backend/internal/gateway/websocket/origin.go b/apps/backend/internal/gateway/websocket/origin.go new file mode 100644 index 0000000000..409fd85cfc --- /dev/null +++ b/apps/backend/internal/gateway/websocket/origin.go @@ -0,0 +1,23 @@ +package websocket + +import ( + "net/http" + + "github.com/kandev/kandev/internal/common/httpmw" +) + +// checkWebSocketOrigin validates the Origin header on WebSocket upgrade +// requests to prevent cross-site WebSocket hijacking: without it, any web +// page the user visits could open a socket to the local backend and drive +// session/shell actions on the host. +// +// Requests without an Origin header are allowed (non-browser clients such as +// the CLI or curl); everything else defers to the shared origin trust policy +// in httpmw.AllowedOrigin, the same policy the CORS middleware enforces. +func checkWebSocketOrigin(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true + } + return httpmw.AllowedOrigin(origin, r.Host) +} diff --git a/apps/backend/internal/gateway/websocket/origin_test.go b/apps/backend/internal/gateway/websocket/origin_test.go new file mode 100644 index 0000000000..76103f7cfe --- /dev/null +++ b/apps/backend/internal/gateway/websocket/origin_test.go @@ -0,0 +1,40 @@ +package websocket + +import ( + "net/http" + "testing" +) + +// The exhaustive origin policy table lives in internal/common/httpmw; this +// covers the upgrade-specific wrapper semantics. +func TestCheckWebSocketOrigin(t *testing.T) { + tests := []struct { + name string + origin string + host string + want bool + }{ + {"no origin allows non-browser clients", "", "example.com", true}, + {"same hostname", "https://example.com", "example.com:8080", true}, + {"loopback cross port", "http://localhost:3000", "127.0.0.1:8080", true}, + {"foreign origin", "https://evil.com", "example.com", false}, + {"malformed origin", "not-a-url", "example.com", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &http.Request{ + Header: http.Header{}, + Host: tt.host, + } + if tt.origin != "" { + r.Header.Set("Origin", tt.origin) + } + + if got := checkWebSocketOrigin(r); got != tt.want { + t.Errorf("checkWebSocketOrigin(origin=%q, host=%q) = %v, want %v", + tt.origin, tt.host, got, tt.want) + } + }) + } +} diff --git a/apps/backend/internal/gateway/websocket/terminal_handler.go b/apps/backend/internal/gateway/websocket/terminal_handler.go index c0d61923ac..bf1f7e80a9 100644 --- a/apps/backend/internal/gateway/websocket/terminal_handler.go +++ b/apps/backend/internal/gateway/websocket/terminal_handler.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "strings" "github.com/gin-gonic/gin" @@ -51,48 +50,6 @@ var terminalUpgrader = gorillaws.Upgrader{ CheckOrigin: checkWebSocketOrigin, } -// checkWebSocketOrigin validates the Origin header for WebSocket connections. -// This prevents cross-site WebSocket hijacking attacks. -func checkWebSocketOrigin(r *http.Request) bool { - origin := r.Header.Get("Origin") - if origin == "" { - // No origin header - allow (could be a non-browser client) - return true - } - - // Allow localhost origins for development - if strings.HasPrefix(origin, "http://localhost") || - strings.HasPrefix(origin, "http://127.0.0.1") || - strings.HasPrefix(origin, "https://localhost") || - strings.HasPrefix(origin, "https://127.0.0.1") { - return true - } - - // Check same-origin: Origin should match the Host header - host := r.Host - if host == "" { - host = r.URL.Host - } - - // Parse the origin URL to get its host - originURL, err := url.Parse(origin) - if err != nil { - return false - } - - // Compare hosts (ignoring port for flexibility) - originHost := originURL.Hostname() - requestHost := host - if colonIdx := strings.LastIndex(requestHost, ":"); colonIdx != -1 { - // Strip port from host if present (but be careful with IPv6) - if !strings.Contains(requestHost, "]") || colonIdx > strings.Index(requestHost, "]") { - requestHost = requestHost[:colonIdx] - } - } - - return originHost == requestHost -} - type terminalRoute struct { kind string id string diff --git a/apps/backend/internal/gateway/websocket/terminal_handler_test.go b/apps/backend/internal/gateway/websocket/terminal_handler_test.go index 8bc9756c78..0ae1c92bdd 100644 --- a/apps/backend/internal/gateway/websocket/terminal_handler_test.go +++ b/apps/backend/internal/gateway/websocket/terminal_handler_test.go @@ -19,62 +19,6 @@ import ( "github.com/kandev/kandev/internal/events/bus" ) -func TestCheckWebSocketOrigin(t *testing.T) { - tests := []struct { - name string - origin string - host string - want bool - }{ - // No origin — allow (non-browser client) - {"no origin", "", "example.com", true}, - - // Localhost variants - {"http://localhost", "http://localhost", "localhost", true}, - {"http://localhost:3000", "http://localhost:3000", "localhost:8080", true}, - {"https://localhost", "https://localhost", "localhost", true}, - {"http://127.0.0.1", "http://127.0.0.1", "127.0.0.1", true}, - {"http://127.0.0.1:3000", "http://127.0.0.1:3000", "127.0.0.1:8080", true}, - {"https://127.0.0.1", "https://127.0.0.1", "127.0.0.1", true}, - - // Same-origin (origin host matches request host) - {"same origin", "https://example.com", "example.com", true}, - {"same origin with port", "https://example.com:443", "example.com:8080", true}, - - // Cross-origin — reject - {"cross origin", "https://evil.com", "example.com", false}, - {"cross origin similar", "https://notexample.com", "example.com", false}, - - // Malformed origin - {"malformed origin", "not-a-url", "example.com", false}, - - // IPv6 host - {"ipv6 cross origin", "http://[::1]:3000", "example.com:8080", false}, - - // Empty host — no match possible - {"empty host rejects", "https://example.com", "", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := &http.Request{ - Header: http.Header{}, - Host: tt.host, - URL: &url.URL{Host: tt.host}, - } - if tt.origin != "" { - r.Header.Set("Origin", tt.origin) - } - - got := checkWebSocketOrigin(r) - if got != tt.want { - t.Errorf("checkWebSocketOrigin(origin=%q, host=%q) = %v, want %v", - tt.origin, tt.host, got, tt.want) - } - }) - } -} - func TestStripTerminalResponses(t *testing.T) { tests := []struct { name string