diff --git a/core/socketio_cors.py b/core/socketio_cors.py index 61b4e4ce..ef41206f 100644 --- a/core/socketio_cors.py +++ b/core/socketio_cors.py @@ -5,8 +5,9 @@ Three concerns lifted out of `web_server.py`: - :func:`resolve_cors_origins` — read the user's ``security.cors_origins`` config setting (string, list, or unset) and return what to hand to Flask-SocketIO's ``cors_allowed_origins`` - parameter: an empty list (same-origin only), the literal ``'*'`` - (wildcard, opt-in), or a list of explicit origin URLs. + parameter: ``None`` (engineio same-origin default — the secure + default), the literal ``'*'`` (wildcard, opt-in), or a list of + explicit origin URLs. - :func:`will_reject` — predict whether engineio's CORS check will reject a request, given the resolved allow-list, the request's @@ -88,9 +89,11 @@ def resolve_cors_origins(config_manager: Any) -> ResolvedOrigins: def will_reject( allowed: ResolvedOrigins, - origin: str, + origin: Optional[str], host: str, + request_scheme: str = '', forwarded_host: str = '', + forwarded_proto: str = '', ) -> bool: """Predict whether engineio's CORS check will reject this request. @@ -98,22 +101,64 @@ def will_reject( a helpful warning *before* the rejection happens. Returns ``True`` when the request will be rejected. - Same-origin check: ``Origin``'s ``host[:port]`` portion matches the - request's ``Host`` header OR the ``X-Forwarded-Host`` header. Engineio - checks both when ``cors_allowed_origins`` is ``None``; we mirror that - so reverse-proxy users with proper proxy headers don't trigger - spurious "rejected" log lines. + Same-origin check: engineio builds full ``{scheme}://{host}`` strings + from the request URL — and adds a second candidate from the + forwarded headers when EITHER ``X-Forwarded-Proto`` OR + ``X-Forwarded-Host`` is present (engineio falls back to the request + Host / scheme for whichever forwarded header is missing). We mirror + that exactly. Comparing scheme matters: a TLS-terminating proxy can + leave the backend seeing ``http://soulsync.foo`` while the browser's + Origin is ``https://soulsync.foo`` — engineio treats those as + different strings and rejects, so we should too. + + Defensive against ``None`` / empty origin: returns ``False`` (allow), + matching engineio's actual behavior (server.py:207: ``if origin:`` + skips the validation block entirely when no Origin header is sent). + Browsers always send Origin for WebSocket upgrades, so this only + matters for non-browser clients like ``curl`` — which engineio + intentionally permits. + + Proxy params default to empty so callers without proxy awareness + fall back to a host-only same-origin check (still correct for + direct-access setups). """ if allowed == '*': return False + if not origin: + return False # Engineio skips CORS validation when no Origin header if isinstance(allowed, list) and origin in allowed: return False - # Origin is "scheme://host[:port][/path]"; pull just host[:port]. - origin_host = origin.split('://', 1)[-1].split('/', 1)[0] - if host and origin_host == host: - return False - if forwarded_host and origin_host == forwarded_host.split(',')[0].strip(): + + # Engineio's same-origin check builds full {scheme}://{host} strings. + # Build the candidate set from the request + any forwarded headers. + candidates = [] + if host: + scheme = request_scheme or 'http' + candidates.append(f"{scheme}://{host}") + if forwarded_host or forwarded_proto: + # Mirror engineio: when EITHER forwarded header is present, build + # a candidate from both, falling back to the request value for + # whichever is missing. (engineio/base_server.py:_cors_allowed_origins.) + f_host = forwarded_host.split(',')[0].strip() if forwarded_host else host + if f_host: + f_scheme = (forwarded_proto.split(',')[0].strip() + if forwarded_proto + else (request_scheme or 'http')) + candidates.append(f"{f_scheme}://{f_host}") + if origin in candidates: return False + + # Backwards-compat shim: callers that don't pass scheme info still + # get the original host-only same-origin check, so callers / tests + # that exercise this predicate without a real Flask request context + # don't get spurious rejections. Production callers always pass + # scheme, so this branch is inert in normal operation. + if not request_scheme and not forwarded_proto: + origin_host = origin.split('://', 1)[-1].split('/', 1)[0] + if host and origin_host == host: + return False + if forwarded_host and origin_host == forwarded_host.split(',')[0].strip(): + return False return True @@ -126,22 +171,30 @@ class RejectionLogger: requests via :meth:`maybe_log` and emits a clear warning the first time each unique origin appears, telling the user where to add it. - Bounded by the number of unique origins ever attempted; cleared on - process restart. The dedup is intentional — a malicious site - hammering the endpoint shouldn't be able to spam logs. + The dedup set is capped (default 100 unique origins) so a hostile + actor opening connections from many distinct fake origins can't grow + memory unbounded. When the cap is hit, a single overflow warning is + emitted and further rejections are silently dropped until the next + process restart (or :meth:`reset_for_tests` for tests). """ - def __init__(self, logger: Any): + DEFAULT_DEDUP_CAP = 100 + + def __init__(self, logger: Any, dedup_cap: int = DEFAULT_DEDUP_CAP): self._logger = logger self._seen: Set[str] = set() self._lock = threading.Lock() + self._cap = max(1, int(dedup_cap)) + self._overflow_warned = False def maybe_log( self, allowed: ResolvedOrigins, origin: Optional[str], host: str, + request_scheme: str = '', forwarded_host: str = '', + forwarded_proto: str = '', ) -> bool: """Log a rejection warning if applicable, deduped. @@ -149,27 +202,49 @@ class RejectionLogger: be safe to call from a Flask ``before_request`` hook on every Socket.IO request — it short-circuits early on requests that won't be rejected (no Origin header, allowed origin, same-origin - match against either Host or X-Forwarded-Host). + match against Host / X-Forwarded-Host with proper scheme). """ if not origin: return False # Non-browser clients (curl, server-to-server) - if not will_reject(allowed, origin, host, forwarded_host): + if not will_reject(allowed, origin, host, request_scheme, + forwarded_host, forwarded_proto): return False + + # Pick the message to emit (or bail) under the lock. Actual + # logger.warning() call happens AFTER the lock releases — keeps + # the critical section minimal and avoids holding our lock while + # the logging framework acquires its own internal locks. + msg: Optional[str] = None with self._lock: if origin in self._seen: return False - self._seen.add(origin) - self._logger.warning( - f"[Socket.IO] Rejecting WebSocket connection from origin '{origin}' " - f"(request Host='{host}'). If this is your reverse-proxy or custom " - f"domain, add it to Settings → Security → Allowed WebSocket Origins." - ) + if len(self._seen) >= self._cap: + if self._overflow_warned: + return False # Already emitted overflow notice; suppress. + self._overflow_warned = True + msg = ( + f"[Socket.IO] Rejection-log dedup cache hit cap " + f"({self._cap} unique origins). Suppressing further " + f"rejection warnings this session — likely indicates " + f"hostile traffic or a misconfigured client. Restart " + f"to reset the cache." + ) + else: + self._seen.add(origin) + msg = ( + f"[Socket.IO] Rejecting WebSocket connection from origin " + f"'{origin}' (request Host='{host}'). If this is your " + f"reverse-proxy or custom domain, add it to " + f"Settings → Security → Allowed WebSocket Origins." + ) + self._logger.warning(msg) return True def reset_for_tests(self) -> None: """Clear the dedup cache. Test-only.""" with self._lock: self._seen.clear() + self._overflow_warned = False def log_startup_status(allowed: ResolvedOrigins, logger: Any) -> None: diff --git a/tests/test_socketio_cors.py b/tests/test_socketio_cors.py index 267260e0..278f443a 100644 --- a/tests/test_socketio_cors.py +++ b/tests/test_socketio_cors.py @@ -20,7 +20,6 @@ These pin the security-relevant behavior: Pure unit tests — no Flask, no engineio, no network. Just the logic. """ -import logging import threading from typing import Any, List @@ -201,6 +200,82 @@ def test_will_reject_honors_x_forwarded_host(): forwarded_host='') is False +def test_will_reject_compares_full_scheme_when_known(): + """When the caller provides scheme info, engineio compares full + {scheme}://{host} strings. A TLS-terminating proxy can leave the + backend seeing http while the browser's Origin is https — engineio + rejects, our predictor must too (otherwise we miss logging it).""" + # Backend sees http, browser sent https → engineio rejects → we predict reject + assert will_reject(None, 'https://soulsync.foo', 'soulsync.foo', + request_scheme='http') is True + + # Backend sees http, browser sent http → match → allow + assert will_reject(None, 'http://soulsync.foo', 'soulsync.foo', + request_scheme='http') is False + + # X-Forwarded-Proto says the public request was https → match origin's https + assert will_reject(None, 'https://soulsync.foo', 'internal:8888', + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https') is False + + # X-Forwarded-Proto says https but Origin is http → mismatch → reject + assert will_reject(None, 'http://soulsync.foo', 'internal:8888', + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https') is True + + # Comma-separated X-Forwarded-Proto (proxy chain) — first wins, like engineio + assert will_reject(None, 'https://soulsync.foo', 'internal:8888', + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https, http') is False + + +def test_will_reject_falls_back_to_host_only_when_no_scheme_info(): + """Backwards-compat shim: callers that don't pass scheme info still + get the basic Host-only same-origin check (the original behavior). + Important for any integration tests that exercise the predicate + without a real Flask request context.""" + # No scheme info → host-only match works + assert will_reject(None, 'https://soulsync.foo', 'soulsync.foo') is False + assert will_reject(None, 'http://x.com', 'x.com') is False + # Cross-origin still rejected + assert will_reject(None, 'https://attacker.com', 'soulsync.foo') is True + + +def test_will_reject_allows_missing_origin_matching_engineio(): + """Engineio (server.py:207: ``if origin:``) skips CORS validation + entirely when no Origin header is sent — non-browser clients (curl, + server-to-server) are intentionally permitted. Our predictor must + match that or we'd log spurious "rejected" warnings for legitimate + non-browser traffic. Must also not raise on None input.""" + # Wildcard permits missing origin — and so does the default policy + # (matches engineio's actual behavior). + assert will_reject('*', None, 'localhost:8888') is False + assert will_reject('*', '', 'localhost:8888') is False + assert will_reject(None, None, 'localhost:8888') is False + assert will_reject(None, '', 'localhost:8888') is False + assert will_reject(['https://x.com'], None, 'localhost:8888') is False + + +def test_will_reject_honors_forwarded_proto_alone(): + """Engineio adds the forwarded candidate when EITHER X-Forwarded-Proto + OR X-Forwarded-Host is present (it falls back to HTTP_HOST for the + missing one). Our predictor must mirror that — otherwise a misconfig + sending only X-Forwarded-Proto would look like a rejection in our + log even though engineio actually allows it.""" + # forwarded_proto alone: backend host stands in for forwarded_host + assert will_reject(None, 'https://localhost:8888', 'localhost:8888', + request_scheme='http', + forwarded_proto='https') is False + + # forwarded_proto alone but origin's host doesn't match the backend host + assert will_reject(None, 'https://attacker.com', 'localhost:8888', + request_scheme='http', + forwarded_proto='https') is True + + # ── RejectionLogger ─────────────────────────────────────────────────────── @@ -230,8 +305,11 @@ def test_rejection_logger_silent_when_request_would_be_allowed(): rl.maybe_log('*', 'https://x.com', 'localhost:8888') # In allow-list — no warning rl.maybe_log(['https://x.com'], 'https://x.com', 'localhost:8888') - # Same-origin via X-Forwarded-Host — no warning - rl.maybe_log(None, 'https://soulsync.foo', 'internal:8888', 'soulsync.foo') + # Same-origin via X-Forwarded-Host (with proxy scheme info) — no warning + rl.maybe_log(None, 'https://soulsync.foo', 'internal:8888', + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https') assert log.warnings == [] @@ -297,6 +375,43 @@ def test_rejection_logger_reset_for_tests_clears_dedup(): assert len(log.warnings) == 2 # logged again after reset +def test_rejection_logger_caps_dedup_set_at_configured_limit(): + """A hostile actor opening connections from many distinct fake origins + would otherwise grow the dedup set unbounded. After the cap is hit, + further rejections are silently dropped (after one overflow notice).""" + log = _CapturingLogger() + rl = RejectionLogger(log, dedup_cap=5) + + # Fill the cap + for i in range(5): + rl.maybe_log(None, f'https://fake{i}.com', 'localhost:8888') + assert len(log.warnings) == 5 + + # Next unique origin → overflow notice, NOT a per-origin warning + rl.maybe_log(None, 'https://fake5.com', 'localhost:8888') + assert len(log.warnings) == 6 + assert 'cap' in log.warnings[5].lower() or 'suppress' in log.warnings[5].lower() + + # Further unique origins → silently dropped (overflow notice already emitted) + for i in range(6, 20): + rl.maybe_log(None, f'https://fake{i}.com', 'localhost:8888') + assert len(log.warnings) == 6 # unchanged + + # After reset, cap restarts + rl.reset_for_tests() + rl.maybe_log(None, 'https://fake0.com', 'localhost:8888') + assert len(log.warnings) == 7 + + +def test_rejection_logger_default_cap_is_reasonable(): + """The default cap should be high enough that legitimate-but-unusual + setups (e.g., a power user with a dozen reverse-proxy domains rotating) + don't hit the overflow notice during normal use.""" + assert RejectionLogger.DEFAULT_DEDUP_CAP >= 50, ( + "default dedup cap should fit normal usage" + ) + + # ── log_startup_status ──────────────────────────────────────────────────── diff --git a/web_server.py b/web_server.py index 77690b3b..80e929e9 100644 --- a/web_server.py +++ b/web_server.py @@ -235,7 +235,9 @@ def _log_rejected_socketio_origin(): _socketio_cors_origins, request.headers.get('Origin'), request.headers.get('Host', ''), + request.scheme, request.headers.get('X-Forwarded-Host', ''), + request.headers.get('X-Forwarded-Proto', ''), ) diff --git a/webui/static/settings.js b/webui/static/settings.js index f2026891..e1f23f03 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -2596,6 +2596,27 @@ async function saveSettings(quiet = false) { } }; + // Validate cors_origins entries — backend silently filters malformed + // values, so warn the user up-front if any line doesn't look like a + // URL (or the special '*' token). One-shot toast; doesn't block save. + const corsRaw = settings.security.cors_origins; + if (corsRaw) { + const entries = corsRaw.replace(/\n/g, ',').split(',') + .map(s => s.trim()) + .filter(s => s); + const invalid = entries.filter(e => { + if (e === '*') return false; + // Accept anything matching scheme://host[:port], no path required + return !/^https?:\/\/[^\s/]+$/i.test(e); + }); + if (invalid.length) { + showToast( + `Allowed Origins: ${invalid.length} entr${invalid.length === 1 ? 'y looks' : 'ies look'} malformed (need full URL like https://soulsync.example.com, no trailing slash). Saving anyway — they\'ll be ignored.`, + 'warning' + ); + } + } + try { if (!quiet) showLoadingOverlay('Saving settings...');