From 3a105b58ccf8e92e60d120c2a95edee0545bbfb5 Mon Sep 17 00:00:00 2001 From: Sudhir Pola Date: Tue, 1 Sep 2026 10:22:57 +0530 Subject: [PATCH] fix: go fmt and copilot fixes fix(cors): reject cross-origin relay handshakes and drop wildcard default The KVM/SOL/IDER relay accepted websocket connections from any origin, enabling Cross-Site WebSocket Hijacking, and allowed_origins defaulted to "*", so the API answered every request with Access-Control-Allow-Origin: *. - Validate the relay's Origin header against the configured allowlist. - Do not honor "*" for the relay: a wildcard or empty allowlist degrades to same-origin only, which closes the hijack on installs that still carry "*" on disk. Same-origin is always accepted, so the embedded UI is unaffected. Opaque origins ("null", data:, file:) are rejected. - Compare hosts case-insensitively, matching the CORS middleware. A mixed-case entry previously passed CORS but failed the relay. - Replace the "*" allowed_headers with an explicit list, and reject an empty allowed_origins at startup instead of panicking in the CORS library. - Remove the wildcard from the shipped config.yml and .env.example, and warn at startup if it is still configured. Deployments serving the UI from a separate origin while relying on "*" must now list that origin explicitly. Same-origin deployments are unaffected. Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .env.example | 14 +- cmd/app/tray_windows.go | 6 +- config/config.go | 39 ++++- config/config.yml | 24 ++- config/config_test.go | 52 ++++++- internal/app/app.go | 104 ++++++++++++- internal/app/app_test.go | 171 +++++++++++++++++++++ internal/controller/ws/v1/redirect.go | 19 ++- internal/controller/ws/v1/redirect_test.go | 127 +++++++++++++++ internal/usecase/sqldb/device.go | 4 +- 10 files changed, 533 insertions(+), 27 deletions(-) diff --git a/.env.example b/.env.example index 53c0459b5..3917bfc0a 100644 --- a/.env.example +++ b/.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. diff --git a/cmd/app/tray_windows.go b/cmd/app/tray_windows.go index a07ae0f2a..a4a182423 100644 --- a/cmd/app/tray_windows.go +++ b/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. // diff --git a/config/config.go b/config/config.go index 1b7f7cc1e..dfa752e9f 100644 --- a/config/config.go +++ b/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 } diff --git a/config/config.yml b/config/config.yml index 782bd309e..2f1769d4d 100644 --- a/config/config.yml +++ b/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: diff --git a/config/config_test.go b/config/config_test.go index 2bf87c7c0..e343f37c3 100644 --- a/config/config_test.go +++ b/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) +} diff --git a/internal/app/app.go b/internal/app/app.go index 2c48620b5..cb35073f9 100644 --- a/internal/app/app.go +++ b/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) { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 16d6f9433..a5be2ca5a 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -85,6 +85,177 @@ func TestSetupHTTPHandlerSetsNoSniffAheadOfCORS(t *testing.T) { } } +func TestNewOriginChecker(t *testing.T) { + t.Parallel() + + allowed := []string{"https://allowed.example"} + + tests := []struct { + name string + // allow defaults to `allowed` when nil. + allow []string + // host overrides the request Host, i.e. what counts as same-origin. + host string + origin string + want bool + }{ + {name: "explicit origin is allowed", origin: "https://allowed.example", want: true}, + {name: "different origin is rejected", origin: "https://evil.example", want: false}, + {name: "missing origin is allowed for non-browser clients", origin: "", want: true}, + + // A wildcard must not reopen the relay to arbitrary sites; it degrades + // to same-origin only. Covers the shipped `allowed_origins: ["*"]` that + // pre-existing installs still carry on disk. + {name: "wildcard rejects a cross-origin page", allow: []string{"*"}, origin: "https://evil.example", want: false}, + {name: "wildcard still allows same-origin", allow: []string{"*"}, host: "console.example:8181", origin: "https://console.example:8181", want: true}, + {name: "empty allowlist rejects a cross-origin page", allow: []string{}, origin: "https://evil.example", want: false}, + {name: "empty allowlist still allows same-origin", allow: []string{}, host: "console.example:8181", origin: "https://console.example:8181", want: true}, + + // Opaque origins: a sandboxed iframe sends the literal "null", and + // data:/file: pages send no usable scheme://host pair either. + {name: "null origin is rejected", origin: "null", want: false}, + {name: "opaque non-URL origin is rejected", origin: "evil.example", want: false}, + {name: "scheme-only origin is rejected", origin: "https://", want: false}, + + // url.Parse lowercases the scheme but not the host, while + // gin-contrib/cors lowercases both. A mixed-case allowlist entry must + // not pass CORS and then fail the relay. + {name: "mixed-case allowlist entry matches lowercase origin", allow: []string{"https://Allowed.Example"}, origin: "https://allowed.example", want: true}, + {name: "mixed-case origin matches lowercase allowlist entry", origin: "https://ALLOWED.example", want: true}, + {name: "mixed-case same-origin host matches", host: "Console.Example:8181", origin: "https://console.example:8181", want: true}, + + // The Origin header carries no path, but a trailing slash in config is + // an easy mistake to make. + {name: "trailing slash in allowlist entry is tolerated", allow: []string{"https://allowed.example/"}, origin: "https://allowed.example", want: true}, + + // Scheme and port are part of the origin: neither may be substituted. + {name: "scheme mismatch is rejected", origin: "http://allowed.example", want: false}, + {name: "port mismatch is rejected", allow: []string{"https://allowed.example:8181"}, origin: "https://allowed.example:4200", want: false}, + {name: "suffix of an allowed host is rejected", origin: "https://evil-allowed.example", want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + allow := tc.allow + if allow == nil { + allow = allowed + } + + checker := newOriginChecker(allow) + req := httptest.NewRequest(http.MethodGet, "/relay/webrelay.ashx", http.NoBody) + + if tc.host != "" { + req.Host = tc.host + } + + if tc.origin != "" { + req.Header.Set("Origin", tc.origin) + } + + require.Equal(t, tc.want, checker(req)) + }) + } +} + +// With an explicit allowlist the CORS middleware must echo the requesting +// origin rather than "*", and must advertise Vary: Origin so a shared cache +// cannot hand one site's response to another. +// +//nolint:paralleltest // setupHTTPHandler mutates global gin mode and reads GIN_MODE, which TestRun writes +func TestSetupHTTPHandlerEchoesOriginAndVaries(t *testing.T) { + cfg := &config.Config{} + cfg.AllowedOrigins = []string{"https://allowed.example"} + cfg.AllowedHeaders = []string{"Content-Type", "Authorization"} + cfg.AllowCredentials = true + cfg.Disabled = true + + prev := config.ConsoleConfig + + t.Cleanup(func() { config.ConsoleConfig = prev }) + + config.ConsoleConfig = cfg + + handler := setupHTTPHandler(cfg, logger.New("error"), &usecase.Usecases{}) + + req := httptest.NewRequest(http.MethodGet, "/healthz", http.NoBody) + req.Header.Set("Origin", "https://allowed.example") + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + require.Equal(t, "https://allowed.example", w.Header().Get("Access-Control-Allow-Origin")) + require.NotEqual(t, "*", w.Header().Get("Access-Control-Allow-Origin")) + require.Contains(t, w.Header().Values("Vary"), "Origin") + require.Equal(t, "true", w.Header().Get("Access-Control-Allow-Credentials")) +} + +// Browsers send an Origin header on same-origin state-changing requests too, so +// an allowlist that names only localhost must not 403 a Console reached at its +// own LAN address. gin-contrib/cors exempts same-origin before validating, and +// newOriginChecker mirrors that for the relay. +// +//nolint:paralleltest // setupHTTPHandler mutates global gin mode and reads GIN_MODE, which TestRun writes +func TestSetupHTTPHandlerAllowsSameOriginOutsideAllowlist(t *testing.T) { + cfg := &config.Config{} + cfg.AllowedOrigins = []string{"https://localhost:8181"} + cfg.AllowedHeaders = []string{"Content-Type"} + cfg.AllowCredentials = true + cfg.Disabled = true + + prev := config.ConsoleConfig + + t.Cleanup(func() { config.ConsoleConfig = prev }) + + config.ConsoleConfig = cfg + + handler := setupHTTPHandler(cfg, logger.New("error"), &usecase.Usecases{}) + + req := httptest.NewRequest(http.MethodGet, "/healthz", http.NoBody) + req.Host = "10.20.224.56:8181" + req.Header.Set("Origin", "https://10.20.224.56:8181") + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code) + + // And the relay accepts the matching websocket handshake. + require.True(t, newOriginChecker(cfg.AllowedOrigins)(req)) +} + +// A wildcard allowlist forbids credentials: the browser refuses the pairing, so +// leaving Access-Control-Allow-Credentials on would only break cookie auth +// while advertising it. +// +//nolint:paralleltest // setupHTTPHandler mutates global gin mode and reads GIN_MODE, which TestRun writes +func TestSetupHTTPHandlerDropsCredentialsUnderWildcard(t *testing.T) { + cfg := &config.Config{} + cfg.AllowedOrigins = []string{"*"} + cfg.AllowedHeaders = []string{"Content-Type"} + cfg.AllowCredentials = true + cfg.Disabled = true + + prev := config.ConsoleConfig + + t.Cleanup(func() { config.ConsoleConfig = prev }) + + config.ConsoleConfig = cfg + + handler := setupHTTPHandler(cfg, logger.New("error"), &usecase.Usecases{}) + + req := httptest.NewRequest(http.MethodGet, "/healthz", http.NoBody) + req.Header.Set("Origin", "https://evil.example") + + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + require.Equal(t, "*", w.Header().Get("Access-Control-Allow-Origin")) + require.Empty(t, w.Header().Get("Access-Control-Allow-Credentials")) +} + func TestRun(t *testing.T) { t.Parallel() diff --git a/internal/controller/ws/v1/redirect.go b/internal/controller/ws/v1/redirect.go index 3efb95a98..48a505289 100644 --- a/internal/controller/ws/v1/redirect.go +++ b/internal/controller/ws/v1/redirect.go @@ -42,16 +42,25 @@ func (r *RedirectRoutes) websocketHandler(c *gin.Context) { return } - upgrader, ok := r.u.(*websocket.Upgrader) - if !ok { - r.l.Debug("failed to cast Upgrader to *websocket.Upgrader") + // Negotiate the caller's token as the subprotocol. A single upgrader is + // shared by every relay handshake, so copy it per request instead of + // mutating it in place: an in-place write races concurrent handshakes and + // makes them negotiate no subprotocol at all, which browsers reject. + // websocket.Upgrader is pure configuration, so copying by value is safe and + // keeps WriteBufferPool shared. + upgrader := r.u + + if shared, ok := r.u.(*websocket.Upgrader); ok { + perRequest := *shared + perRequest.Subprotocols = []string{tokenString} + upgrader = &perRequest } else { - upgrader.Subprotocols = []string{tokenString} + r.l.Debug("failed to cast Upgrader to *websocket.Upgrader") } // KVM_TIMING: Measure WebSocket upgrade duration upgradeStart := time.Now() - conn, err := r.u.Upgrade(c.Writer, c.Request, nil) + conn, err := upgrader.Upgrade(c.Writer, c.Request, nil) upgradeDuration := time.Since(upgradeStart) devices.RecordWebsocketUpgrade(upgradeDuration) r.l.Debug("KVM_TIMING: WebSocket upgrade", "duration_ms", upgradeDuration.Milliseconds()) diff --git a/internal/controller/ws/v1/redirect_test.go b/internal/controller/ws/v1/redirect_test.go index 235aa709f..96839a43d 100644 --- a/internal/controller/ws/v1/redirect_test.go +++ b/internal/controller/ws/v1/redirect_test.go @@ -2,8 +2,13 @@ package v1 import ( "errors" + "fmt" + "io" + "log" "net/http" "net/http/httptest" + "strings" + "sync" "testing" "time" @@ -11,6 +16,7 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/gorilla/websocket" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "github.com/device-management-toolkit/console/config" @@ -229,6 +235,127 @@ func TestWebSocketHandlerDeviceBinding(t *testing.T) { //nolint:paralleltest // }) } +// TestWebSocketHandlerRealUpgrader exercises the *websocket.Upgrader branch the +// mock-based tests above never reach, over a real TCP handshake. A single +// upgrader is shared by every relay request, so the handler must set +// Subprotocols on a per-request copy: mutating the shared one in place is a +// data race (caught by -race in the concurrent subtest) and lets one handshake +// negotiate against another's token, which yields no subprotocol at all and is +// rejected by browsers. +func TestWebSocketHandlerRealUpgrader(t *testing.T) { //nolint:paralleltest // shared config and logger + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + _, _ = config.NewConfig() + + config.ConsoleConfig.Disabled = false + config.ConsoleConfig.JWTKey = "test-jwt-key" + + tokenFor := func(deviceID string) string { + claims := jwt.MapClaims{ + "exp": time.Now().Add(5 * time.Minute).Unix(), + "deviceId": deviceID, + } + + s, _ := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(config.ConsoleConfig.JWTKey)) + + return s + } + + mockLogger := mocks.NewMockLogger(ctrl) + mockLogger.EXPECT().Debug(gomock.Any(), gomock.Any()).AnyTimes() + mockLogger.EXPECT().Info(gomock.Any(), gomock.Any()).AnyTimes() + + // The handshake response is already flushed by the time Redirect runs, so + // closing here just releases the hijacked connection. + mockFeature := mocks.NewMockDeviceManagementFeature(ctrl) + mockFeature.EXPECT(). + Redirect(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(_ *gin.Context, conn *websocket.Conn, _, _ string) error { + return conn.Close() + }). + AnyTimes() + + // Shared by every handshake, exactly as app.setupHTTPHandler wires it. + upgrader := &websocket.Upgrader{ReadBufferSize: 1024, WriteBufferSize: 1024} + + r := gin.New() + RegisterRoutes(r, mockLogger, mockFeature, upgrader) + + srv := httptest.NewUnstartedServer(r) + // Writing the gin response onto a hijacked connection is expected and noisy. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + + t.Cleanup(srv.Close) + + wsURL := "ws" + strings.TrimPrefix(srv.URL, "http") + + // dial performs a real handshake as deviceID does, returning the subprotocol + // the server negotiated and the token the client offered. + dial := func(deviceID string) (negotiated, offered string, err error) { + offered = tokenFor(deviceID) + dialer := &websocket.Dialer{Subprotocols: []string{offered}} + + conn, resp, err := dialer.Dial(wsURL+"/relay/webrelay.ashx?host="+deviceID+"&mode=kvm", nil) + if err != nil { + return "", offered, err + } + + defer conn.Close() + + if resp != nil { + defer resp.Body.Close() + } + + return conn.Subprotocol(), offered, nil + } + + t.Run("negotiates the caller's token as the subprotocol", func(t *testing.T) { //nolint:paralleltest // shared server + negotiated, offered, err := dial("deviceA") + + require.NoError(t, err) + assert.Equal(t, offered, negotiated) + }) + + t.Run("leaves the shared upgrader untouched", func(t *testing.T) { //nolint:paralleltest // shared server + _, _, err := dial("deviceB") + + require.NoError(t, err) + assert.Nil(t, upgrader.Subprotocols, "handshake must not mutate the shared upgrader") + }) + + // The regression case: run under -race, concurrent in-place mutation of the + // shared upgrader is reported, and handshakes negotiate each other's tokens. + t.Run("concurrent handshakes negotiate their own subprotocol", func(t *testing.T) { //nolint:paralleltest // shared server + const handshakes = 16 + + var wg sync.WaitGroup + + results := make([]struct { + negotiated, offered string + err error + }, handshakes) + + for i := range results { + wg.Add(1) + + go func(i int) { + defer wg.Done() + + results[i].negotiated, results[i].offered, results[i].err = dial(fmt.Sprintf("device-%d", i)) + }(i) + } + + wg.Wait() + + for i, res := range results { + require.NoErrorf(t, res.err, "handshake %d failed", i) + assert.Equalf(t, res.offered, res.negotiated, "handshake %d negotiated another caller's subprotocol", i) + } + }) +} + // TestWebSocketHandlerTokenValidation: WS rejects missing and unverifiable tokens. func TestWebSocketHandlerTokenValidation(t *testing.T) { //nolint:paralleltest // logging library is not thread-safe for tests ctrl := gomock.NewController(t) diff --git a/internal/usecase/sqldb/device.go b/internal/usecase/sqldb/device.go index 7766d3523..a47351cea 100644 --- a/internal/usecase/sqldb/device.go +++ b/internal/usecase/sqldb/device.go @@ -241,15 +241,13 @@ func (r *DeviceRepo) GetByTags(_ context.Context, tags []string, method string, "deviceinfo"). From("devices") - var params []interface{} - if method == "AND" { // All tags must be present (simulating an 'AND' operation) for _, tag := range tags { builder = builder.Where("(',' || tags || ',') LIKE ? AND tenantId = ?", "%,"+tag+",%", tenantID) - params = append(params, "%,"+tag+",%", tenantID) //nolint:staticcheck // intentionally retained; the AND branch passes its args to Where inline } } else { + var params []interface{} // Any tag is present (simulating an 'OR' operation) var conditions []string for _, tag := range tags {