From 27854a6da54e57b097f2fd0df7492deb397d5215 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:46:17 +0100 Subject: [PATCH] Stop websocket upgrade headers exempting demo-mode writes DemoModeMiddleware admitted any request carrying "Upgrade: websocket", whatever its method or path, so a POST/PUT/PATCH/DELETE with that header reached its handler on a read-only demo instance. The public demo signs visitors in as the configured admin user, which RequireAdmin accepts and which skips token scope checks, so the middleware was the only write guard: adding two headers let a demo session switch the instance out of mock mode, change system settings or create organisations. The exemption never served a real handshake. RFC 6455 handshakes are GET requests, gorilla's Upgrader refuses any other method, and GET already passes the read-only branch above it, so /ws and /api/agent/ws keep connecting. Removing the branch leaves the method alone deciding. TestContract_DemoModeUpgradeHeadersDoNotExemptWrites signs in as the demo admin through the full router, proves the session passes RequireAdmin, gets 403 for a mock-mode POST with upgrade headers and still opens /ws. On the old middleware the same POST returned 200 and turned mock mode off. --- .../v6/internal/subsystems/agent-lifecycle.md | 9 ++ .../v6/internal/subsystems/api-contracts.md | 11 ++ .../internal/subsystems/storage-recovery.md | 11 ++ internal/api/contract_test.go | 114 ++++++++++++++++++ internal/api/demo_middleware.go | 11 +- internal/api/demo_middleware_test.go | 83 ++++++++++++- 6 files changed, 230 insertions(+), 9 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 21f4060e21..57249df605 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8880,3 +8880,12 @@ agent from its last report instead of from the PVE poll, and mark it stale at their own heartbeat cutoff (`fleethealth.AgentStaleThreshold`, which is separate from the monitoring reporting lease). Agent registration, enrolment, install, update, removal and report identity are unchanged. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or +`DELETE` carrying `Upgrade: websocket` is judged by the demo read-only guard +like any other write instead of always reaching its handler. Agent command +websocket handshakes to `/api/agent/ws` are HTTP/1.1 `GET` requests, the only +transport its gorilla upgrader serves, and still connect on a demo instance. +No agent registration, enrolment, install, update or removal path changed. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 978b841439..9daacf4b40 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -7645,6 +7645,17 @@ read-only demo account. That same hidden read-side boundary includes `GET` and `HEAD` reads for `/api/admin/users` and manual discovery at `/api/discover`; public demo mode may block writes generically, but it must not reveal that admin-user inventory or manual-discovery read routes exist. +That generic write block keys on the request method alone. The demo +middleware admits `GET`, `HEAD` and `OPTIONS` plus the login, logout, AI +execute and OIDC login/callback allowlist; HTTP/1.1 websocket handshakes for +`/ws` and `/api/agent/ws` are `GET` requests and connect through that read +branch, so an `Upgrade: websocket` or `Connection: Upgrade` header never +carries a `POST`, `PUT`, `PATCH` or `DELETE` past the guard. A demo deployment that signs +visitors in as the configured admin, as the public demo does, has only this +guard between that session and admin writes such as `/api/system/mock-mode`. +`TestContract_DemoModeUpgradeHeadersDoNotExemptWrites` in +`internal/api/contract_test.go` signs in as that admin through the full router +and pins both halves. That shared infrastructure install boundary now also preserves copied shell command payload continuity: any privilege-escalation wrapper applied at `frontend-modern/src/components/Settings/InfrastructureInstallerSection.tsx` diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 66185fb88d..b38a046d36 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6860,3 +6860,14 @@ migration or recovery authority is added or moved. The performance report and reporting runtime snapshot handlers apply the same test before tabulating a disk temperature. That changes only which held value a report shows, not any storage or recovery path. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` no longer exempts every request carrying +`Upgrade: websocket` from the demo read-only guard. On a `DEMO_MODE` instance a +`POST`, `PUT`, `PATCH` or `DELETE` with websocket handshake headers is now +judged like any other write: outside the login, logout, AI execute and OIDC +allowlist it gets the generic demo `403`, or `404` on a hidden route, and that +includes config import at `/api/config/import`. HTTP/1.1 websocket handshakes +are `GET` requests and still connect. No storage, retention, backup, migration +or recovery path is added or moved. diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 1adcbc7d63..953ed0fba2 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -60,6 +60,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/internal/updates" "github.com/rcourtman/pulse-go-rewrite/internal/vmware" + pulsews "github.com/rcourtman/pulse-go-rewrite/internal/websocket" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" "github.com/rcourtman/pulse-go-rewrite/pkg/aicontracts" "github.com/rcourtman/pulse-go-rewrite/pkg/audit" @@ -9261,6 +9262,119 @@ func TestContract_DemoModeCommercialSurfacePolicy(t *testing.T) { }) } +// The public demo signs every visitor in as the configured admin, so admin +// routes accept a demo session and the demo guard is the only thing between it +// and a write. Websocket handshake headers must not carry a write past that +// guard, while a genuine handshake from the same session must still connect. +func TestContract_DemoModeUpgradeHeadersDoNotExemptWrites(t *testing.T) { + setMockModeForTest(t, true) + + dataDir := t.TempDir() + hashedPass, err := authpkg.HashPassword("demo") + if err != nil { + t.Fatalf("hash password: %v", err) + } + cfg := &config.Config{ + DataPath: dataDir, + ConfigPath: dataDir, + DemoMode: true, + AuthUser: "demo", + AuthPass: hashedPass, + } + + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + t.Cleanup(func() { monitor.Stop() }) + if err := monitor.SetMockMode(true); err != nil { + t.Fatalf("set monitor mock mode: %v", err) + } + + hub := pulsews.NewHub(nil) + go hub.Run() + t.Cleanup(hub.Stop) + router := NewRouter(cfg, monitor, nil, hub, nil, "1.0.0") + cleanupTestRouter(t, router) + server := newIPv4HTTPServer(t, router.Handler()) + t.Cleanup(server.Close) + + loginResp, err := http.Post(server.URL+"/api/login", "application/json", strings.NewReader(`{"username":"demo","password":"demo"}`)) + if err != nil { + t.Fatalf("demo login: %v", err) + } + loginResp.Body.Close() + if loginResp.StatusCode != http.StatusOK { + t.Fatalf("demo login status = %d, want %d", loginResp.StatusCode, http.StatusOK) + } + var cookiePairs []string + csrfToken := "" + for _, cookie := range loginResp.Cookies() { + cookiePairs = append(cookiePairs, cookie.Name+"="+cookie.Value) + if cookie.Name == CookieNameCSRF { + csrfToken = cookie.Value + } + } + if csrfToken == "" { + t.Fatal("expected CSRF cookie after demo login") + } + sessionCookies := strings.Join(cookiePairs, "; ") + + send := func(method, path, body string, headers map[string]string) (int, string) { + t.Helper() + req, err := http.NewRequest(method, server.URL+path, strings.NewReader(body)) + if err != nil { + t.Fatalf("build %s %s: %v", method, path, err) + } + req.Header.Set("Cookie", sessionCookies) + for key, value := range headers { + req.Header.Set(key, value) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, path, err) + } + defer resp.Body.Close() + var payload bytes.Buffer + _, _ = payload.ReadFrom(resp.Body) + return resp.StatusCode, payload.String() + } + + if status, body := send(http.MethodGet, "/api/system/mock-mode", "", nil); status != http.StatusOK { + t.Fatalf("demo session read of admin route status = %d, want %d: %s", status, http.StatusOK, body) + } + + status, body := send(http.MethodPost, "/api/system/mock-mode", `{"enabled":false}`, map[string]string{ + "Content-Type": "application/json", + "X-CSRF-Token": csrfToken, + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", + }) + if status != http.StatusForbidden || !strings.Contains(body, "Demo mode enabled") { + t.Fatalf("demo write with websocket upgrade headers status = %d, want %d demo block: %s", status, http.StatusForbidden, body) + } + if !mock.IsMockEnabled() { + t.Fatal("demo write with websocket upgrade headers switched mock mode off") + } + + wsHeaders := wsHeadersForHTTP(t, server.URL) + wsHeaders.Set("Cookie", sessionCookies) + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+"/ws", wsHeaders) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("demo session websocket handshake failed: %v (status %d)", err, status) + } + defer conn.Close() + if resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("demo session websocket handshake status = %d, want %d", resp.StatusCode, http.StatusSwitchingProtocols) + } +} + func TestContract_ReleaseDemoFixtureRuntimeGuardrailsRemainCanonical(t *testing.T) { requiredSnippets := map[string][]string{ "licensing_handlers.go": { diff --git a/internal/api/demo_middleware.go b/internal/api/demo_middleware.go index 7b55a8bf7c..3584529b27 100644 --- a/internal/api/demo_middleware.go +++ b/internal/api/demo_middleware.go @@ -33,18 +33,15 @@ func DemoModeMiddleware(cfg *config.Config, next http.Handler) http.Handler { } } - // Allow GET and HEAD requests (read-only) + // Allow GET and HEAD requests (read-only). WebSocket handshakes are GET + // requests (RFC 6455 section 4.1), so /ws and /api/agent/ws connect + // through this branch. An Upgrade header never exempts another method: + // a POST carrying "Upgrade: websocket" is still a write. if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodOptions { next.ServeHTTP(w, r) return } - // Allow WebSocket upgrades - if strings.ToLower(r.Header.Get("Upgrade")) == "websocket" { - next.ServeHTTP(w, r) - return - } - // Allow authentication endpoints (login is read-only - verifies credentials) authPaths := []string{ "/api/login", diff --git a/internal/api/demo_middleware_test.go b/internal/api/demo_middleware_test.go index 3ed38b032f..b4afd8aa97 100644 --- a/internal/api/demo_middleware_test.go +++ b/internal/api/demo_middleware_test.go @@ -4,8 +4,11 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" + "time" + "github.com/gorilla/websocket" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -44,8 +47,8 @@ func TestDemoModeMiddleware(t *testing.T) { {"demo on websocket GET", true, http.MethodGet, "/api/ws", "websocket", true, http.StatusOK, true}, {"demo on websocket case insensitive", true, http.MethodGet, "/api/ws", "WebSocket", true, http.StatusOK, true}, {"demo on websocket uppercase", true, http.MethodGet, "/api/ws", "WEBSOCKET", true, http.StatusOK, true}, - // WebSocket upgrade with POST method (tests websocket branch after GET/HEAD/OPTIONS check) - {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", true, http.StatusOK, true}, + // An Upgrade header does not turn a write into a websocket handshake + {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", false, http.StatusForbidden, true}, // Demo mode enabled - auth endpoints allowed (POST) {"demo on login", true, http.MethodPost, "/api/login", "", true, http.StatusOK, true}, @@ -172,3 +175,79 @@ func TestDemoModeMiddleware_BlockedResponse(t *testing.T) { t.Error("message should not be empty") } } + +// A write that carries websocket handshake headers is still a write. The demo +// guard used to wave through any request with "Upgrade: websocket", so a demo +// session could switch mock mode off or change settings by adding two headers. +func TestDemoModeMiddlewareUpgradeHeadersDoNotExemptWrites(t *testing.T) { + handler := DemoModeMiddleware(&config.Config{DemoMode: true}, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("write with websocket upgrade headers reached the handler: %s %s", r.Method, r.URL.Path) + })) + + methods := []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} + paths := []string{"/api/system/mock-mode", "/api/system/settings/update", "/api/orgs", "/ws", "/api/agent/ws"} + for _, method := range methods { + for _, path := range paths { + req := httptest.NewRequest(method, path, strings.NewReader(`{"enabled":false}`)) + req.Header.Set("Connection", "Upgrade") + req.Header.Set("Upgrade", "websocket") + req.Header.Set("Sec-WebSocket-Version", "13") + req.Header.Set("Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ==") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Errorf("%s %s with websocket upgrade headers: status = %d, want %d", method, path, rec.Code, http.StatusForbidden) + } + } + } +} + +func TestDemoModeMiddlewareAllowsWebSocketHandshake(t *testing.T) { + upgrader := websocket.Upgrader{CheckOrigin: func(*http.Request) bool { return true }} + echo := func(w http.ResponseWriter, r *http.Request) { + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer conn.Close() + messageType, payload, err := conn.ReadMessage() + if err != nil { + return + } + _ = conn.WriteMessage(messageType, payload) + } + mux := http.NewServeMux() + mux.HandleFunc("/ws", echo) + mux.HandleFunc("/api/agent/ws", echo) + + server := newIPv4HTTPServer(t, DemoModeMiddleware(&config.Config{DemoMode: true}, mux)) + defer server.Close() + + for _, path := range []string{"/ws", "/api/agent/ws"} { + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+path, nil) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("websocket handshake to %s failed in demo mode: %v (status %d)", path, err, status) + } + if resp.StatusCode != http.StatusSwitchingProtocols { + conn.Close() + t.Fatalf("websocket handshake to %s: status = %d, want %d", path, resp.StatusCode, http.StatusSwitchingProtocols) + } + if err := conn.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil { + conn.Close() + t.Fatalf("set read deadline for %s: %v", path, err) + } + if err := conn.WriteMessage(websocket.TextMessage, []byte("ping")); err != nil { + conn.Close() + t.Fatalf("write to %s: %v", path, err) + } + _, payload, err := conn.ReadMessage() + conn.Close() + if err != nil || string(payload) != "ping" { + t.Fatalf("echo from %s = %q, %v; want %q", path, payload, err, "ping") + } + } +}