From ec0ebeda2f2547fe02e7bd57e805eab907e79056 Mon Sep 17 00:00:00 2001 From: Kpa-clawbot Date: Mon, 29 Jun 2026 05:48:58 -0700 Subject: [PATCH] fix(#1793): WebSocket CheckOrigin allowlist (block cross-origin scrapers) (#1795) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Closes the wide-open `/ws` WebSocket upgrader (`CheckOrigin: return true`) that lets any browser origin scrape live packet data. Replaces it with an explicit allowlist consulted from `cfg.CORSAllowedOrigins`, plus an implicit same-origin allowance and an empty-Origin (non-browser client) allowance. Fixes #1793. ## Rules (`Hub.checkOrigin`) - Empty `Origin` header → **allow** (non-browser clients; per-IP rate/deny gating tracked separately in #1794). - `Origin` host == request `Host` (case-insensitive) → **allow** (same-origin). - `Origin` matches an entry in `cfg.CORSAllowedOrigins` by exact case-insensitive match → **allow**. - `"*"` in `cfg.CORSAllowedOrigins` is **deliberately ignored** for `/ws`. A startup `[ws] WARNING:` is logged once when present. - Anything else → **reject** (gorilla returns 403). ### Deliberate divergence from CORS XHR CORS XHR (`corsMiddleware`) still honors `"*"` for read-only cross-origin GETs. The `/ws` upgrade does NOT, per OWASP's WebSocket Security Cheat Sheet: > Use an allowlist, not a denylist. Avoid wildcards or substring matching. — https://cheatsheetseries.owasp.org/cheatsheets/WebSocket_Security_Cheat_Sheet.html `"*"` on the WS path would re-open the exact CSWSH/scraping vector this PR closes, so it is rejected with a startup warning rather than silently honored. This intentional asymmetry is documented in the updated `_comment_corsAllowedOrigins` in `config.example.json`. ## TDD red → green - `e5974c6a` **RED** — adds `cmd/server/websocket_checkorigin_test.go` with five cases; `SetAllowedOrigins` introduced as an enforcement stub so the test compiles and fails on the assertion (CI fails on this commit by design). - `a4791dc3` **GREEN** — implements `Hub.checkOrigin`, wires `SetAllowedOrigins` from `main.go`, updates the config example. All tests pass. ## Tests added (`cmd/server/websocket_checkorigin_test.go`) - `TestCheckOriginRejectsForeignOrigin` — foreign Origin → 403 - `TestCheckOriginAllowsEmptyOrigin` — non-browser client → 101 - `TestCheckOriginAllowsSameHost` — same-origin → 101 - `TestCheckOriginAllowsAllowlistedOrigin` — exact allowlist match → 101 - `TestCheckOriginWildcardDoesNotAllowForeignOrigin` — `"*"` in allowlist still rejects foreign origin → 403 ## Files changed - `cmd/server/websocket.go` — `Hub.allowedOrigins`, `SetAllowedOrigins`, `checkOrigin`, wired into `Upgrader.CheckOrigin`. - `cmd/server/main.go` — `hub.SetAllowedOrigins(cfg.CORSAllowedOrigins)` at the single call site. - `cmd/server/websocket_checkorigin_test.go` — new test file. - `config.example.json` — updated `_comment_corsAllowedOrigins` to document `/ws` gating and the `"*"` divergence. ## Out of scope (follow-up) - **#1794** — per-IP rate limit / deny list / connection cap for non-browser clients (which still bypass Origin because they don't send one). Layered defense; not in this PR. ## Verification - `go test ./cmd/server/...` — all server tests pass locally (574s). - Preflight clean (`bash ~/.openclaw/skills/pr-preflight/scripts/run-all.sh origin/master`). --------- Co-authored-by: openclaw-bot --- cmd/server/main.go | 1 + cmd/server/websocket.go | 74 ++++++++++++-- cmd/server/websocket_checkorigin_test.go | 120 +++++++++++++++++++++++ config.example.json | 2 +- 4 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 cmd/server/websocket_checkorigin_test.go diff --git a/cmd/server/main.go b/cmd/server/main.go index 083ed14a..f467295d 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -321,6 +321,7 @@ func main() { // WebSocket hub hub := NewHub() + hub.SetAllowedOrigins(cfg.CORSAllowedOrigins) hub.upgrader.EnableCompression = cfg.WSCompressionEnabled() // HTTP server diff --git a/cmd/server/websocket.go b/cmd/server/websocket.go index c9ba1aae..ba4adc90 100644 --- a/cmd/server/websocket.go +++ b/cmd/server/websocket.go @@ -4,6 +4,7 @@ import ( "encoding/json" "log" "net/http" + "net/url" "strings" "sync" "time" @@ -13,9 +14,63 @@ import ( // Hub manages WebSocket clients and broadcasts. type Hub struct { - mu sync.RWMutex - clients map[*Client]bool - upgrader websocket.Upgrader + mu sync.RWMutex + clients map[*Client]bool + upgrader websocket.Upgrader + allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) +} + +// SetAllowedOrigins configures the exact-match origin allowlist consulted by +// the WebSocket upgrader's CheckOrigin. The "*" wildcard is deliberately NOT +// honored here (it IS honored by the HTTP CORS middleware): OWASP's +// WebSocket Security Cheat Sheet recommends an explicit allowlist for CSWSH +// defense. If "*" appears in the slice, it is ignored and a startup WARN is +// logged once per call. +// +// See: https://cheatsheetseries.owasp.org/cheatsheets/WebSocket_Security_Cheat_Sheet.html +func (h *Hub) SetAllowedOrigins(origins []string) { + h.mu.Lock() + defer h.mu.Unlock() + h.allowedOrigins = append(h.allowedOrigins[:0], origins...) + for _, o := range origins { + if o == "*" { + log.Println(`[ws] WARNING: CORSAllowedOrigins contains "*" — CORS allows any origin for XHR, but /ws upgrade enforces explicit allowlist only (OWASP CSWSH guidance). Add specific origins to allow cross-origin WebSocket clients.`) + break + } + } +} + +// checkOrigin is the gorilla/websocket Upgrader.CheckOrigin hook. Rules: +// - empty Origin header → allow (non-browser client; rate-limit / IP gate +// is handled separately, see #1794). +// - Origin host == request Host (same-origin) → allow. +// - Origin in allowedOrigins by exact case-insensitive match → allow. +// - "*" in allowedOrigins is ignored (see SetAllowedOrigins). +// - anything else → reject (gorilla returns 403). +func (h *Hub) checkOrigin(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true + } + u, err := url.Parse(origin) + if err != nil { + return false + } + if strings.EqualFold(u.Host, r.Host) { + return true + } + h.mu.RLock() + allowed := h.allowedOrigins + h.mu.RUnlock() + for _, o := range allowed { + if o == "*" { + continue // deliberately not honored — see SetAllowedOrigins + } + if strings.EqualFold(o, origin) { + return true + } + } + return false } // Client is a single WebSocket connection. @@ -26,14 +81,15 @@ type Client struct { } func NewHub() *Hub { - return &Hub{ + h := &Hub{ clients: make(map[*Client]bool), - upgrader: websocket.Upgrader{ - ReadBufferSize: 1024, - WriteBufferSize: 4096, - CheckOrigin: func(r *http.Request) bool { return true }, - }, } + h.upgrader = websocket.Upgrader{ + ReadBufferSize: 1024, + WriteBufferSize: 4096, + CheckOrigin: h.checkOrigin, + } + return h } func (h *Hub) ClientCount() int { diff --git a/cmd/server/websocket_checkorigin_test.go b/cmd/server/websocket_checkorigin_test.go new file mode 100644 index 00000000..b9b999dc --- /dev/null +++ b/cmd/server/websocket_checkorigin_test.go @@ -0,0 +1,120 @@ +package main + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gorilla/websocket" +) + +// dialWS attempts a WebSocket upgrade against srv with the given Origin +// header. It returns the HTTP status code received (101 on success, 403 on +// rejection) and any dial error. Connection is closed if it succeeds. +func dialWS(t *testing.T, srv *httptest.Server, origin string) (int, error) { + t.Helper() + wsURL := "ws" + srv.URL[4:] + headers := http.Header{} + if origin != "" { + headers.Set("Origin", origin) + } + conn, resp, err := websocket.DefaultDialer.Dial(wsURL, headers) + if conn != nil { + defer conn.Close() + } + status := 0 + if resp != nil { + status = resp.StatusCode + } + return status, err +} + +func newCheckOriginServer(t *testing.T, allowed []string) (*httptest.Server, *Hub) { + t.Helper() + hub := NewHub() + hub.SetAllowedOrigins(allowed) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hub.ServeWS(w, r) + })) + return srv, hub +} + +// TestCheckOriginRejectsForeignOrigin: the RED test that proves a +// cross-origin browser cannot upgrade /ws when not allowlisted. +// (#1793) +func TestCheckOriginRejectsForeignOrigin(t *testing.T) { + srv, _ := newCheckOriginServer(t, nil) + defer srv.Close() + + status, err := dialWS(t, srv, "https://evil.example.com") + if err == nil { + t.Fatalf("expected upgrade rejection for foreign origin, got success (status=%d)", status) + } + if status != http.StatusForbidden { + t.Fatalf("expected 403 Forbidden for foreign origin, got status=%d err=%v", status, err) + } +} + +func TestCheckOriginAllowsEmptyOrigin(t *testing.T) { + srv, _ := newCheckOriginServer(t, nil) + defer srv.Close() + + status, err := dialWS(t, srv, "") + if err != nil { + t.Fatalf("expected upgrade success for empty Origin (non-browser client), got status=%d err=%v", status, err) + } + if status != http.StatusSwitchingProtocols { + t.Fatalf("expected 101 Switching Protocols, got %d", status) + } +} + +func TestCheckOriginAllowsSameHost(t *testing.T) { + srv, _ := newCheckOriginServer(t, nil) + defer srv.Close() + + // httptest.NewServer uses srv.URL host, e.g. 127.0.0.1:PORT. + hostURL := "http" + srv.URL[4:] // strip "ws" was N/A — srv.URL is http:// + if !strings.HasPrefix(srv.URL, "http://") { + t.Fatalf("unexpected srv URL: %s", srv.URL) + } + sameOrigin := hostURL[:len(hostURL)] // same scheme+host as the dial target + status, err := dialWS(t, srv, sameOrigin) + if err != nil { + t.Fatalf("expected upgrade success for same-host origin %s, got status=%d err=%v", sameOrigin, status, err) + } + if status != http.StatusSwitchingProtocols { + t.Fatalf("expected 101 Switching Protocols, got %d", status) + } +} + +func TestCheckOriginAllowsAllowlistedOrigin(t *testing.T) { + allow := []string{"https://embed.example.com"} + srv, _ := newCheckOriginServer(t, allow) + defer srv.Close() + + status, err := dialWS(t, srv, "https://embed.example.com") + if err != nil { + t.Fatalf("expected upgrade success for allowlisted origin, got status=%d err=%v", status, err) + } + if status != http.StatusSwitchingProtocols { + t.Fatalf("expected 101 Switching Protocols, got %d", status) + } +} + +// TestCheckOriginWildcardDoesNotAllowForeignOrigin asserts the deliberate +// divergence from CORS: "*" in the allowlist does NOT permit cross-origin +// WebSocket upgrades. OWASP WebSocket Security Cheat Sheet — avoid wildcards +// in CSWSH defense. See PR for citation. +func TestCheckOriginWildcardDoesNotAllowForeignOrigin(t *testing.T) { + srv, _ := newCheckOriginServer(t, []string{"*"}) + defer srv.Close() + + status, err := dialWS(t, srv, "https://evil.example.com") + if err == nil { + t.Fatalf("expected upgrade rejection for foreign origin even when allowlist=[\"*\"], got success (status=%d)", status) + } + if status != http.StatusForbidden { + t.Fatalf("expected 403 Forbidden, got status=%d err=%v", status, err) + } +} diff --git a/config.example.json b/config.example.json index 13e53b50..b5c0f55b 100644 --- a/config.example.json +++ b/config.example.json @@ -34,7 +34,7 @@ }, "_comment_ingestorStats": "Ingestor publishes a 1-Hz stats snapshot consumed by the server's /api/perf/io and /api/perf/write-sources endpoints (#1120). Path is configured via the CORESCOPE_INGESTOR_STATS environment variable on the INGESTOR process. Default: /tmp/corescope-ingestor-stats.json. The writer uses O_NOFOLLOW + 0o600, so a pre-planted symlink in /tmp cannot be used to clobber an arbitrary file. SECURITY: in shared-tmp environments (multi-tenant hosts), point CORESCOPE_INGESTOR_STATS at a private directory like /var/lib/corescope/ingestor-stats.json that only the corescope user can write to.", "corsAllowedOrigins": [], - "_comment_corsAllowedOrigins": "Cross-origin allowlist for embed scenarios (#1369). Exact-match origins, e.g. [\"https://blog.example.com\", \"https://embed.example.com\"]. When empty (default), no Access-Control-* headers are sent and browsers enforce same-origin. When non-empty, only the listed origins receive CORS headers, and Access-Control-Allow-Methods is limited to GET, HEAD, OPTIONS (the cross-domain surface is read-only — same-origin admin writes are unaffected). Use [\"*\"] to allow any origin (NOT recommended for write-capable deployments). Operators can override per-deployment with the CORS_ALLOWED_ORIGINS environment variable (comma-separated). No credentialed CORS is enabled. To embed the map or channels pages cross-domain, add the embedding origin here and use the URL pattern '/#/map?embed=1' or '/#/channels?embed=1' — embed mode hides the top-nav, bottom-nav, and side drawer for full-bleed iframe rendering.", + "_comment_corsAllowedOrigins": "Cross-origin allowlist for embed scenarios (#1369) AND for /ws WebSocket upgrades (#1793 — CSWSH defense per OWASP WebSocket Security Cheat Sheet). Exact-match origins, e.g. [\"https://blog.example.com\", \"https://embed.example.com\"]. Same-origin requests (Origin host == request Host) and non-browser clients (no Origin header) are always allowed for /ws. When empty (default), no Access-Control-* headers are sent and browsers enforce same-origin; cross-origin /ws upgrades are rejected. When non-empty, only the listed origins receive CORS headers, and Access-Control-Allow-Methods is limited to GET, HEAD, OPTIONS (the cross-domain surface is read-only — same-origin admin writes are unaffected). Use [\"*\"] to allow any origin for CORS XHR (NOT recommended for write-capable deployments); note that \"*\" is deliberately NOT honored for /ws upgrades — list explicit origins to permit cross-origin WebSocket clients (OWASP guidance: avoid wildcards in CSWSH defense). Operators can override per-deployment with the CORS_ALLOWED_ORIGINS environment variable (comma-separated). No credentialed CORS is enabled. To embed the map or channels pages cross-domain, add the embedding origin here and use the URL pattern '/#/map?embed=1' or '/#/channels?embed=1' — embed mode hides the top-nav, bottom-nav, and side drawer for full-bleed iframe rendering.", "https": { "cert": "/path/to/cert.pem", "key": "/path/to/key.pem",