diff --git a/core/socketio_cors.py b/core/socketio_cors.py index ef41206f..425a464e 100644 --- a/core/socketio_cors.py +++ b/core/socketio_cors.py @@ -76,7 +76,9 @@ def resolve_cors_origins(config_manager: Any) -> ResolvedOrigins: return None parts = [p.strip() for p in raw.replace('\n', ',').split(',')] elif isinstance(raw, (list, tuple)): - parts = [str(p).strip() for p in raw] + # Drop non-string entries instead of stringifying — `[None]` would + # otherwise coerce to ``['None']`` and become a junk allow-list entry. + parts = [p.strip() for p in raw if isinstance(p, str)] else: return None parts = [p for p in parts if p] @@ -118,9 +120,10 @@ def will_reject( 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). + ``request_scheme`` is required for an accurate same-origin match — + engineio compares full ``{scheme}://{host}`` strings, so callers + that omit it default to ``'http'``. Production wires Flask's + ``request.scheme`` here, which WSGI guarantees to be non-empty. """ if allowed == '*': return False @@ -145,21 +148,7 @@ def will_reject( 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 + return origin not in candidates class RejectionLogger: @@ -184,7 +173,10 @@ class RejectionLogger: self._logger = logger self._seen: Set[str] = set() self._lock = threading.Lock() - self._cap = max(1, int(dedup_cap)) + try: + self._cap = max(1, int(dedup_cap)) + except (TypeError, ValueError): + self._cap = self.DEFAULT_DEDUP_CAP self._overflow_warned = False def maybe_log( @@ -204,8 +196,6 @@ class RejectionLogger: won't be rejected (no Origin header, allowed origin, same-origin 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, request_scheme, forwarded_host, forwarded_proto): return False diff --git a/tests/test_socketio_cors.py b/tests/test_socketio_cors.py index 278f443a..b54d94d3 100644 --- a/tests/test_socketio_cors.py +++ b/tests/test_socketio_cors.py @@ -139,43 +139,42 @@ def test_resolve_cors_origins_never_silently_returns_wildcard_for_garbage(): # ── will_reject ─────────────────────────────────────────────────────────── -@pytest.mark.parametrize("allowed, origin, host, expected_reject", [ - # Same-origin (Origin's host:port matches the request Host) — allow - (None, 'http://localhost:8888', 'localhost:8888', False), - (None, 'http://192.168.1.5:8888','192.168.1.5:8888',False), - (None, 'https://soulsync.foo', 'soulsync.foo', False), +@pytest.mark.parametrize("allowed, origin, host, scheme, expected_reject", [ + # Same-origin (Origin's full {scheme}://{host} matches request) — allow + (None, 'http://localhost:8888', 'localhost:8888', 'http', False), + (None, 'http://192.168.1.5:8888', '192.168.1.5:8888', 'http', False), + (None, 'https://soulsync.foo', 'soulsync.foo', 'https', False), # Cross-origin with default allow-list — reject - (None, 'https://x.com', 'localhost:8888', True), - (None, 'https://soulsync.foo', 'localhost:8888', True), # reverse proxy NOT forwarding Host + (None, 'https://x.com', 'localhost:8888', 'http', True), + (None, 'https://soulsync.foo', 'localhost:8888', 'http', True), # reverse proxy NOT forwarding Host + # Scheme mismatch — engineio rejects, so do we + (None, 'https://soulsync.foo', 'soulsync.foo', 'http', True), # Wildcard short-circuit — allow - ('*', 'https://x.com', 'localhost:8888', False), - ('*', 'https://anything.evil', 'localhost:8888', False), + ('*', 'https://x.com', 'localhost:8888', 'http', False), + ('*', 'https://anything.evil', 'localhost:8888', 'http', False), # Origin in allow-list — allow - (['https://x.com'], 'https://x.com', 'localhost:8888', False), - (['https://soulsync.foo'], 'https://soulsync.foo', 'localhost:8888', False), + (['https://x.com'], 'https://x.com', 'localhost:8888', 'http', False), + (['https://soulsync.foo'], 'https://soulsync.foo', 'localhost:8888', 'http', False), # Cross-origin not in allow-list — reject - (['https://x.com'], 'https://y.com', 'localhost:8888', True), + (['https://x.com'], 'https://y.com', 'localhost:8888', 'http', True), # Same-origin still works even when allow-list has other entries - (['https://x.com'], 'http://localhost:8888', 'localhost:8888', False), - - # Origin with path component — only host:port should be compared - (None, 'http://x.com:8080/path', 'x.com:8080', False), + (['https://x.com'], 'http://localhost:8888', 'localhost:8888', 'http', False), ]) -def test_will_reject_predicts_engineio_decision(allowed, origin, host, expected_reject): - assert will_reject(allowed, origin, host) is expected_reject +def test_will_reject_predicts_engineio_decision(allowed, origin, host, scheme, expected_reject): + assert will_reject(allowed, origin, host, request_scheme=scheme) is expected_reject def test_will_reject_with_empty_host_only_uses_allowlist(): """If the request somehow has no Host header (shouldn't happen but be safe), same-origin can't be checked — fall through to allow-list only.""" - assert will_reject(None, 'https://x.com', '') is True - assert will_reject(['https://x.com'], 'https://x.com', '') is False - assert will_reject('*', 'https://x.com', '') is False + assert will_reject(None, 'https://x.com', '', request_scheme='https') is True + assert will_reject(['https://x.com'], 'https://x.com', '', request_scheme='https') is False + assert will_reject('*', 'https://x.com', '', request_scheme='https') is False def test_will_reject_honors_x_forwarded_host(): @@ -183,20 +182,27 @@ def test_will_reject_honors_x_forwarded_host(): cors_allowed_origins is None (engineio/base_server.py:_cors_allowed_origins). Our predictor must mirror that — otherwise reverse-proxy users with proper proxy headers would trigger spurious "rejected" log lines.""" - # Same-origin via X-Forwarded-Host (typical reverse-proxy setup) + # Same-origin via X-Forwarded-Host (typical TLS-terminating reverse proxy) assert will_reject(None, 'https://soulsync.foo', 'internal:8888', - forwarded_host='soulsync.foo') is False + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https') is False # X-Forwarded-Host with comma list (proxy chain) — first entry wins assert will_reject(None, 'https://soulsync.foo', 'internal:8888', - forwarded_host='soulsync.foo, edge.proxy') is False + request_scheme='http', + forwarded_host='soulsync.foo, edge.proxy', + forwarded_proto='https') is False # X-Forwarded-Host doesn't match either — still reject assert will_reject(None, 'https://attacker.com', 'internal:8888', - forwarded_host='soulsync.foo') is True + request_scheme='http', + forwarded_host='soulsync.foo', + forwarded_proto='https') is True # X-Forwarded-Host empty — falls back to Host check (the unset case) assert will_reject(None, 'https://soulsync.foo', 'soulsync.foo', + request_scheme='https', forwarded_host='') is False @@ -232,18 +238,6 @@ def test_will_reject_compares_full_scheme_when_known(): 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, diff --git a/web_server.py b/web_server.py index 80e929e9..b3d47be7 100644 --- a/web_server.py +++ b/web_server.py @@ -228,7 +228,13 @@ _plex_pin_requests_lock = threading.Lock() def _log_rejected_socketio_origin(): """Hook the WS upgrade path so users see a clear log line when their Origin is about to be rejected (engineio otherwise just silently 403s - the upgrade). Dedup + threading lives in `core/socketio_cors`.""" + the upgrade). Dedup + threading lives in `core/socketio_cors`. + + Note: Flask's ``before_request`` runs on every HTTP request to every + endpoint — there's no path-scoped equivalent for arbitrary URL + prefixes. We early-return on non-/socket.io/ paths to keep the + overhead to one string compare per request. + """ if not request.path.startswith('/socket.io/'): return _socketio_rejection_logger.maybe_log( diff --git a/webui/static/settings.js b/webui/static/settings.js index e1f23f03..fb47a9ec 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -2606,8 +2606,9 @@ async function saveSettings(quiet = false) { .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); + // Accept scheme://host[:port] only — no path, query, or fragment. + // Engineio compares Origin against {scheme}://{host} exactly. + return !/^https?:\/\/[^\s/?#]+$/i.test(e); }); if (invalid.length) { showToast(