refactor(cors): reject cross-origin relay handshakes and drop wildcard def…

.env.example

@@ -21,12 +21,16 @@ APP_DISABLE_CIRA=true
 HTTP_HOST=
 HTTP_PORT=8181
 WS_COMPRESSION=false
-HTTP_ALLOWED_ORIGINS=*
-HTTP_ALLOWED_HEADERS=*
-# Lets a cross-origin browser send the session cookie. Ignored while
+# Comma-separated origins allowed to make cross-origin calls and to open a
+# redirection (KVM/SOL/IDER) websocket. Add the origin the UI is served from if
+# it is not this server. Never use "*": it exposes every API response to any
+# site, and the relay refuses cross-origin handshakes while it is set.
+HTTP_ALLOWED_ORIGINS=https://localhost:8181,https://127.0.0.1:8181,http://localhost:8181,http://127.0.0.1:8181,http://localhost:4200,http://127.0.0.1:4200,https://localhost:4200,https://127.0.0.1:4200
+HTTP_ALLOWED_HEADERS=Origin,Accept,Content-Type,Content-Length,Authorization,If-Match
+# Lets a cross-origin browser send the session cookie. Required when the UI is
+# served from a different origin than the API; ignored while
 # HTTP_ALLOWED_ORIGINS is "*" (CORS forbids credentials with a wildcard).
-# Same-origin deployments do not need this.
-HTTP_ALLOW_CREDENTIALS=false
+HTTP_ALLOW_CREDENTIALS=true
 
 # TLS
 # Enable TLS in release if the app terminates TLS itself. If behind an API gateway or LB that provides TLS, set to false.

cmd/app/tray_windows.go

@@ -19,8 +19,10 @@ import (
 const detachedProcess = 0x00000008
 
 // CreateMutexW returns ERROR_ALREADY_EXISTS when another instance holds the named mutex.
-const mutexName = "Local\\DMTConsoleTray"
-const errorAlreadyExists uint32 = 183
+const (
+	mutexName                 = "Local\\DMTConsoleTray"
+	errorAlreadyExists uint32 = 183
+)
 
 // ensureSingleInstance prevents concurrent tray processes via a named mutex.
 //

config/config.go

@@ -23,6 +23,7 @@ var TrayMode bool
 var (
 	ErrJWTExpirationInvalid            = errors.New("config: auth.jwtExpiration must be at least 1 minute (e.g. 24h) — very short expirations render tokens unusable")
 	ErrRedirectionJWTExpirationInvalid = errors.New("config: auth.redirectionJWTExpiration must be at least 1 minute (e.g. 5m) — very short expirations render redirection tokens unusable")
+	ErrAllowedOriginsEmpty             = errors.New("config: http.allowed_origins must list at least one origin (e.g. https://localhost:8181) — an empty list disables CORS outright and aborts startup")
 )
 
 const defaultHost = "localhost"
@@ -191,11 +192,33 @@ func defaultConfig() *Config {
 			DisableCIRA:          true,
 		},
 		HTTP: HTTP{
-			Host:             "",
-			Port:             "8181",
-			AllowedOrigins:   []string{"*"},
-			AllowedHeaders:   []string{"*"},
-			AllowCredentials: false,
+			Host: "",
+			Port: "8181",
+			AllowedOrigins: []string{
+				"http://localhost:8181",
+				"http://localhost:4200",
+				"http://127.0.0.1:8181",
+				"http://127.0.0.1:4200",
+				"https://localhost:8181",
+				"https://localhost:4200",
+				"https://127.0.0.1:8181",
+				"https://127.0.0.1:4200",
+			},
+			// Explicit rather than "*": Access-Control-Allow-Headers: * is taken
+			// literally by browsers once credentials are in play, and never
+			// covers Authorization even without them.
+			AllowedHeaders: []string{
+				"Origin",
+				"Accept",
+				"Content-Type",
+				"Content-Length",
+				"Authorization",
+				"If-Match",
+			},
+			// Safe alongside the explicit AllowedOrigins above: setupHTTPHandler
+			// forces this off whenever the allowlist contains "*", so cookie
+			// auth is only ever granted to enumerated origins.
+			AllowCredentials: true,
 			WSCompression:    true,
 			TLS: TLS{
 				Enabled:  true,
@@ -418,6 +441,12 @@ func (c *Config) validate() error {
 		return ErrRedirectionJWTExpirationInvalid
 	}
 
+	// Caught here rather than left to gin-contrib/cors, which panics with an
+	// opaque "conflict settings: all origins disabled" on an empty list.
+	if len(c.AllowedOrigins) == 0 {
+		return ErrAllowedOriginsEmpty
+	}
+
 	return nil
 }
 

config/config.yml

@@ -14,10 +14,30 @@ http:
     # If certFile/keyFile are both empty and enabled is true, a self-signed certificate will be generated at runtime.
     certFile: ""
     keyFile: ""
+  # Origins allowed to make cross-origin calls and to open a redirection
+  # (KVM/SOL/IDER) websocket. Add the origin the UI is served from if it is not
+  # this server. Never use "*": it exposes every API response to any site, and
+  # the relay refuses cross-origin handshakes while it is set.
   allowed_origins:
-    - "*"
+    - "https://localhost:8181"
+    - "https://127.0.0.1:8181"
+    - "http://localhost:8181"
+    - "http://127.0.0.1:8181"
+    # `ng serve` dev UI
+    - "http://localhost:4200"
+    - "https://localhost:4200"
+    - "http://127.0.0.1:4200"
+    - "https://127.0.0.1:4200"
   allowed_headers:
-    - "*"
+    - "Origin"
+    - "Accept"
+    - "Content-Type"
+    - "Content-Length"
+    - "Authorization"
+    - "If-Match"
+  # Lets a cross-origin UI send the HttpOnly session cookie. Forced off while
+  # allowed_origins contains "*", as CORS forbids that combination.
+  allow_credentials: true
 logger:
   log_level: info
 secrets: 

config/config_test.go

@@ -38,8 +38,25 @@ func TestNewConfig_Defaults(t *testing.T) { //nolint:paralleltest // cannot have
 
 	assert.Equal(t, "", cfg.Host)
 	assert.Equal(t, "8181", cfg.Port)
-	assert.Equal(t, []string{"*"}, cfg.AllowedOrigins)
-	assert.Equal(t, []string{"*"}, cfg.AllowedHeaders)
+	assert.Equal(t, []string{
+		"http://localhost:8181",
+		"http://localhost:4200",
+		"http://127.0.0.1:8181",
+		"http://127.0.0.1:4200",
+		"https://localhost:8181",
+		"https://localhost:4200",
+		"https://127.0.0.1:8181",
+		"https://127.0.0.1:4200",
+	}, cfg.AllowedOrigins)
+	assert.Equal(t, []string{
+		"Origin",
+		"Accept",
+		"Content-Type",
+		"Content-Length",
+		"Authorization",
+		"If-Match",
+	}, cfg.AllowedHeaders)
+	assert.Equal(t, true, cfg.AllowCredentials)
 	assert.Equal(t, true, cfg.TLS.Enabled)
 
 	assert.Equal(t, "info", cfg.Level)
@@ -485,3 +502,34 @@ func TestValidate_ValidDefaults(t *testing.T) {
 	err := cfg.validate()
 	require.NoError(t, err)
 }
+
+// Rejected here rather than left to gin-contrib/cors, which panics on an empty
+// AllowOrigins list.
+func TestValidate_EmptyAllowedOrigins(t *testing.T) {
+	t.Parallel()
+
+	for _, origins := range [][]string{nil, {}} {
+		cfg := defaultConfig()
+		cfg.AllowedOrigins = origins
+
+		err := cfg.validate()
+		require.ErrorIs(t, err, ErrAllowedOriginsEmpty)
+	}
+}
+
+// The shipped defaults must not hand every site a CORS pass, and must not use
+// "*" for headers either: browsers read that literally once credentials are in
+// play, and it never covers Authorization.
+func TestDefaultConfig_NoWildcardCORS(t *testing.T) {
+	t.Parallel()
+
+	cfg := defaultConfig()
+
+	require.NotContains(t, cfg.AllowedOrigins, "*")
+	require.NotEmpty(t, cfg.AllowedOrigins)
+	require.NotContains(t, cfg.AllowedHeaders, "*")
+	require.Contains(t, cfg.AllowedHeaders, "Authorization")
+	// Safe only because the origins above are explicit; setupHTTPHandler drops
+	// it if an admin reintroduces "*".
+	require.True(t, cfg.AllowCredentials)
+}

internal/app/app.go

@@ -5,9 +5,11 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"os/signal"
 	"slices"
+	"strings"
 	"syscall"
 
 	"github.com/gin-contrib/cors"
@@ -78,10 +80,17 @@ func setupHTTPHandler(cfg *config.Config, log logger.Interface, usecases *usecas
 	// rejects disallowed origins itself, so anything after it never runs.
 	handler.Use(securityHeaders())
 
+	wildcardOrigin := slices.Contains(cfg.AllowedOrigins, "*")
+	if wildcardOrigin {
+		log.Warn(`http.allowed_origins contains "*": every site may read API responses ` +
+			`(Access-Control-Allow-Origin: *), cookie auth is disabled, and cross-origin ` +
+			`redirection (KVM/SOL/IDER) is refused. List the Console UI origins explicitly.`)
+	}
+
 	defaultConfig := cors.DefaultConfig()
 	defaultConfig.AllowOrigins = cfg.AllowedOrigins
 	defaultConfig.AllowHeaders = cfg.AllowedHeaders
-	defaultConfig.AllowCredentials = cfg.AllowCredentials && !slices.Contains(cfg.AllowedOrigins, "*")
+	defaultConfig.AllowCredentials = cfg.AllowCredentials && !wildcardOrigin
 
 	handler.Use(cors.New(defaultConfig))
 	httpapi.NewRouter(handler, log, *usecases, cfg)
@@ -92,11 +101,13 @@ func setupHTTPHandler(cfg *config.Config, log logger.Interface, usecases *usecas
 		log.Info("pprof enabled at /debug/pprof/")
 	}
 
+	// Subprotocols is deliberately unset: the relay negotiates the caller's
+	// redirection token as the subprotocol, so wsv1 sets it on a per-request
+	// copy of this upgrader. Anything configured here would be overwritten.
 	upgrader := &websocket.Upgrader{
 		ReadBufferSize:    64 * 1024,
 		WriteBufferSize:   64 * 1024,
-		Subprotocols:      []string{"direct"},
-		CheckOrigin:       func(_ *http.Request) bool { return true },
+		CheckOrigin:       newOriginChecker(cfg.AllowedOrigins),
 		EnableCompression: cfg.WSCompression,
 	}
 
@@ -105,6 +116,93 @@ func setupHTTPHandler(cfg *config.Config, log logger.Interface, usecases *usecas
 	return handler
 }
 
+// newOriginChecker builds the websocket.Upgrader CheckOrigin callback guarding
+// the redirection relay (/relay/webrelay.ashx) against Cross-Site WebSocket
+// Hijacking: without it, any page could open a KVM/SOL/IDER relay using a
+// session the browser attaches automatically.
+//
+// Unlike the CORS middleware it deliberately does not honor "*". No site has a
+// legitimate reason to relay someone else's KVM session, so a wildcard
+// allowlist degrades to same-origin only rather than allowing everything —
+// which keeps pre-existing `allowed_origins: ["*"]` installs (the shipped
+// default until now) safe without an admin editing config.yml.
+func newOriginChecker(allowedOrigins []string) func(*http.Request) bool {
+	// If "*" is present, degrade to same-origin only for the relay.
+	if slices.Contains(allowedOrigins, "*") {
+		allowedOrigins = nil
+	}
+
+	normalizedAllowed := normalizeAllowedOrigins(allowedOrigins)
+
+	return func(r *http.Request) bool {
+		origin := r.Header.Get("Origin")
+		// Non-browser clients (rpc-go, CLI tooling, tests) send no Origin at
+		// all. Browsers always send one on a websocket handshake, so allowing
+		// the empty case cannot be used by the hijacking this guards against —
+		// and the JWT in Sec-Websocket-Protocol is still required either way.
+		if origin == "" {
+			return true
+		}
+
+		originURL := parseOrigin(origin)
+		// Opaque origin: "null" from a sandboxed iframe, or a data:/file: page.
+		if originURL == nil {
+			return false
+		}
+
+		// The embedded UI is served by this very server, so same-origin always
+		// passes regardless of what the allowlist says.
+		if strings.EqualFold(originURL.Host, r.Host) {
+			return true
+		}
+
+		return slices.Contains(normalizedAllowed, normalizedFrom(originURL))
+	}
+}
+
+func normalizeAllowedOrigins(allowedOrigins []string) []string {
+	normalizedAllowed := make([]string, 0, len(allowedOrigins))
+	for _, allowedOrigin := range allowedOrigins {
+		if normalizedOrigin := normalizeOrigin(allowedOrigin); normalizedOrigin != "" {
+			normalizedAllowed = append(normalizedAllowed, normalizedOrigin)
+		}
+	}
+
+	return normalizedAllowed
+}
+
+// parseOrigin returns the parsed origin, or nil if it is not a usable
+// scheme://host pair ("", "*", "null", "/some/path").
+func parseOrigin(origin string) *url.URL {
+	if origin == "" {
+		return nil
+	}
+
+	originURL, err := url.Parse(origin)
+	if err != nil || originURL.Scheme == "" || originURL.Host == "" {
+		return nil
+	}
+
+	return originURL
+}
+
+// normalizedFrom reduces a parsed origin to scheme://host, dropping any path
+// and lowercasing the host. url.Parse lowercases the scheme but not the host,
+// and gin-contrib/cors lowercases both — matching it here keeps a mixed-case
+// entry in allowed_origins from passing CORS but failing the relay.
+func normalizedFrom(originURL *url.URL) string {
+	return originURL.Scheme + "://" + strings.ToLower(originURL.Host)
+}
+
+func normalizeOrigin(origin string) string {
+	originURL := parseOrigin(origin)
+	if originURL == nil {
+		return ""
+	}
+
+	return normalizedFrom(originURL)
+}
+
 // securityHeaders sets X-Content-Type-Options: nosniff to stop MIME sniffing.
 func securityHeaders() gin.HandlerFunc {
 	return func(c *gin.Context) {