diff --git a/core/socketio_cors.py b/core/socketio_cors.py new file mode 100644 index 00000000..61b4e4ce --- /dev/null +++ b/core/socketio_cors.py @@ -0,0 +1,191 @@ +"""Socket.IO CORS allow-list resolution + rejection logging. + +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. + +- :func:`will_reject` — predict whether engineio's CORS check will + reject a request, given the resolved allow-list, the request's + ``Origin`` header, and the request's ``Host`` header. Used to log a + helpful warning *before* engineio silently 403s a WebSocket upgrade. + (Without this, the user just sees a half-broken UI with no live + updates and nothing in the logs explaining why.) + +- :class:`RejectionLogger` — threadsafe dedup wrapper around the warning + emitter. Each unique origin is logged once per process so a malicious + site repeatedly hammering the WS endpoint can't spam logs. + +Pure logic, no Flask app dependency. Web_server.py imports these and +wires them into the SocketIO init + a Flask ``before_request`` hook. +""" + +from __future__ import annotations + +import threading +from typing import Any, List, Optional, Set, Union + + +# What ``cors_allowed_origins`` accepts and what we hand to Flask-SocketIO: +# +# - ``None`` → engineio's same-origin default. engineio computes the +# allowed origin list from the request itself: ``scheme://HTTP_HOST`` +# plus ``X-Forwarded-Proto://X-Forwarded-Host`` when those headers are +# present. Reverse proxies that set X-Forwarded-Host (Nginx with +# ``proxy_set_header X-Forwarded-Host`` — and Caddy/Traefik by default) +# work transparently. THE SECURE DEFAULT. +# +# - ``'*'`` → allow any origin. Insecure; opt-in only. +# +# - ``[origin, ...]`` → explicit allow-list. For setups whose Origin +# matches neither the backend's Host nor any forwarded header. +# +# IMPORTANT: do NOT use ``[]``. In engineio that means "disable CORS +# handling entirely" (server.py:202: ``if cors_allowed_origins != []:``) +# which is identical to the ``'*'`` wildcard from a security standpoint. +ResolvedOrigins = Union[List[str], str, None] + + +def resolve_cors_origins(config_manager: Any) -> ResolvedOrigins: + """Resolve the configured Socket.IO allow-list. + + Reads ``security.cors_origins`` from ``config_manager`` and normalizes + whatever shape the user typed (or didn't) into one of three values: + + - ``None`` (the secure default). Hand to Flask-SocketIO and engineio + enforces same-origin, with automatic support for X-Forwarded-Host + so reverse-proxy users don't need to configure anything. + - ``'*'`` — literal wildcard. Allows any origin. Insecure; opt-in. + - ``[origin, ...]`` — list of explicit origin URLs. For users behind + a proxy that doesn't send the forwarded headers OR for custom + contexts (Electron wrappers, browser extensions). + + Accepts the config value as either a string (comma OR newline + separated, since the settings UI is a textarea) or a list. Anything + else falls back to ``None`` — the secure default. + """ + raw = config_manager.get('security.cors_origins', None) if config_manager else None + if raw is None: + return None + if isinstance(raw, str): + if not raw.strip(): + 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] + else: + return None + parts = [p for p in parts if p] + if not parts: + return None + if any(p == '*' for p in parts): + return '*' + return parts + + +def will_reject( + allowed: ResolvedOrigins, + origin: str, + host: str, + forwarded_host: str = '', +) -> bool: + """Predict whether engineio's CORS check will reject this request. + + Mirrors engineio's allow-list / same-origin logic so callers can log + 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. + """ + if allowed == '*': + return False + 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(): + return False + return True + + +class RejectionLogger: + """Threadsafe dedup wrapper that logs each rejected origin only once. + + Engineio silently 403s WebSocket upgrades from disallowed origins. + Without a log line the user sees a half-broken UI (no live progress, + no toasts) and has no idea what's wrong. This class watches incoming + 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. + """ + + def __init__(self, logger: Any): + self._logger = logger + self._seen: Set[str] = set() + self._lock = threading.Lock() + + def maybe_log( + self, + allowed: ResolvedOrigins, + origin: Optional[str], + host: str, + forwarded_host: str = '', + ) -> bool: + """Log a rejection warning if applicable, deduped. + + Returns ``True`` if a warning was emitted this call. Designed to + 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). + """ + if not origin: + return False # Non-browser clients (curl, server-to-server) + if not will_reject(allowed, origin, host, forwarded_host): + return False + 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." + ) + return True + + def reset_for_tests(self) -> None: + """Clear the dedup cache. Test-only.""" + with self._lock: + self._seen.clear() + + +def log_startup_status(allowed: ResolvedOrigins, logger: Any) -> None: + """Emit a one-shot startup log line describing the resolved policy. + + - For ``'*'`` (wildcard) → warning, since it's a security risk. + - For a non-empty list → info, so the user can confirm their config + took effect. + - For ``None`` (same-origin default) → silent. That's the default; + nothing noteworthy. + """ + if allowed == '*': + logger.warning( + "[Socket.IO] cors_allowed_origins is set to '*' — any website can open " + "a WebSocket to this instance. Set Settings → Security → Allowed Origins " + "to a specific list (or leave empty for same-origin only) to lock this down." + ) + elif allowed: + logger.info(f"[Socket.IO] Allowed cross-origin connections from: {allowed}") diff --git a/tests/test_socketio_cors.py b/tests/test_socketio_cors.py new file mode 100644 index 00000000..267260e0 --- /dev/null +++ b/tests/test_socketio_cors.py @@ -0,0 +1,333 @@ +"""Tests for `core.socketio_cors` — the resolver, rejection predictor, +and dedup logger that gate Socket.IO WebSocket origins. + +These pin the security-relevant behavior: + +- The resolver returns ``None`` (engineio's same-origin default — also + the secure default) for anything other than an explicit allow-list or + the wildcard. CRITICAL: the resolver must NEVER return ``[]`` — in + engineio that means "disable CORS handling" which is identical to the + ``'*'`` wildcard from a security standpoint (engineio/server.py:202: + ``if cors_allowed_origins != []``). And it must never silently turn + into ``'*'`` from a misshapen config value. +- The rejection predictor must mirror engineio's same-origin check + exactly so the warning we log is accurate. This includes accepting + matches against ``X-Forwarded-Host`` since engineio honors that + automatically when ``cors_allowed_origins`` is ``None``. +- The dedup logger must emit each unique origin only once so a malicious + site repeatedly hammering the WS endpoint can't spam logs. + +Pure unit tests — no Flask, no engineio, no network. Just the logic. +""" + +import logging +import threading +from typing import Any, List + +import pytest + +from core.socketio_cors import ( + RejectionLogger, + log_startup_status, + resolve_cors_origins, + will_reject, +) + + +# ── helpers ─────────────────────────────────────────────────────────────── + + +class _FakeConfig: + """Minimal config_manager stub that returns one canned value for the + `security.cors_origins` key. Anything else returns the default.""" + + def __init__(self, value: Any): + self._value = value + + def get(self, key: str, default: Any = None) -> Any: + if key == 'security.cors_origins': + return self._value + return default + + +class _CapturingLogger: + """Stand-in logger that records every warning/info call so tests can + assert what was emitted (and how many times).""" + + def __init__(self): + self.warnings: List[str] = [] + self.infos: List[str] = [] + + def warning(self, msg: str) -> None: + self.warnings.append(msg) + + def info(self, msg: str) -> None: + self.infos.append(msg) + + +# ── resolve_cors_origins ────────────────────────────────────────────────── + + +@pytest.mark.parametrize("value, expected", [ + # Unset / empty / whitespace / bogus types → None (engineio same-origin default) + (None, None), + ('', None), + (' ', None), + ('\n\n', None), + (',,,', None), + (12345, None), # numeric — invalid type + ({'a': 1}, None), # dict — invalid type + ([], None), # explicit empty list + ([' ', ''], None), # list of all-empty strings + + # Wildcard + ('*', '*'), + (' * ', '*'), + (['*'], '*'), + (['https://x.com', '*'], '*'), # wildcard in a list still wins + + # Single origin + ('https://x.com', ['https://x.com']), + (['https://x.com'], ['https://x.com']), + + # Multiple origins, comma-separated + ('https://x.com, http://y.com', ['https://x.com', 'http://y.com']), + + # Multiple origins, newline-separated (textarea input) + ('https://x.com\nhttp://y.com', ['https://x.com', 'http://y.com']), + + # Mixed separators + extra commas / whitespace get cleaned + ('https://x.com,, http://y.com,\n http://z.com', ['https://x.com', 'http://y.com', 'http://z.com']), + + # List with mixed types (bytes-like → str coerce) + (['https://x.com', ' ', 'http://y.com'], ['https://x.com', 'http://y.com']), +]) +def test_resolve_cors_origins_normalizes_input(value, expected): + assert resolve_cors_origins(_FakeConfig(value)) == expected + + +def test_resolve_cors_origins_handles_missing_config_manager(): + """Defensive: if config_manager is None (e.g., very early init), the + resolver must fall back to the secure default rather than crashing.""" + assert resolve_cors_origins(None) is None + + +def test_resolve_cors_origins_never_returns_empty_list(): + """SECURITY CRITICAL: ``cors_allowed_origins=[]`` in engineio means + "disable CORS handling entirely" — identical security to ``'*'`` + (engineio/server.py:202). The resolver must return ``None`` for the + secure default, never ``[]``, regardless of what the user typed.""" + edge_cases = [None, '', ' ', '\n\n', ',,,', 12345, 3.14, {'a': 1}, + object(), True, False, [], [' '], ['', ' '], (' ',)] + for value in edge_cases: + result = resolve_cors_origins(_FakeConfig(value)) + assert result != [], ( + f"resolve_cors_origins({value!r}) returned [] — that disables " + f"engineio's CORS check entirely, allowing all origins. Must be None." + ) + + +def test_resolve_cors_origins_never_silently_returns_wildcard_for_garbage(): + """Security-critical: a misshapen config value must NEVER turn into + `'*'` by accident. Anything we can't parse falls back to same-origin.""" + for bogus in [12345, 3.14, {'a': 1}, object(), True, False]: + assert resolve_cors_origins(_FakeConfig(bogus)) is None, ( + f"resolve_cors_origins({bogus!r}) returned a non-None value — " + f"bogus inputs must default to same-origin only" + ) + + +# ── 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), + + # 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 + + # Wildcard short-circuit — allow + ('*', 'https://x.com', 'localhost:8888', False), + ('*', 'https://anything.evil', 'localhost:8888', False), + + # Origin in allow-list — allow + (['https://x.com'], 'https://x.com', 'localhost:8888', False), + (['https://soulsync.foo'], 'https://soulsync.foo', 'localhost:8888', False), + + # Cross-origin not in allow-list — reject + (['https://x.com'], 'https://y.com', 'localhost:8888', 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), +]) +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_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 + + +def test_will_reject_honors_x_forwarded_host(): + """Engineio honors X-Forwarded-Host automatically when + 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) + assert will_reject(None, 'https://soulsync.foo', 'internal:8888', + forwarded_host='soulsync.foo') 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 + + # X-Forwarded-Host doesn't match either — still reject + assert will_reject(None, 'https://attacker.com', 'internal:8888', + forwarded_host='soulsync.foo') is True + + # X-Forwarded-Host empty — falls back to Host check (the unset case) + assert will_reject(None, 'https://soulsync.foo', 'soulsync.foo', + forwarded_host='') is False + + +# ── RejectionLogger ─────────────────────────────────────────────────────── + + +def test_rejection_logger_emits_once_per_unique_origin(): + log = _CapturingLogger() + rl = RejectionLogger(log) + + # Same origin three times — only one warning + for _ in range(3): + rl.maybe_log(None, 'https://attacker.com', 'localhost:8888') + assert len(log.warnings) == 1 + assert 'attacker.com' in log.warnings[0] + + # Different origin — separate warning + rl.maybe_log(None, 'https://other.evil', 'localhost:8888') + assert len(log.warnings) == 2 + assert 'other.evil' in log.warnings[1] + + +def test_rejection_logger_silent_when_request_would_be_allowed(): + log = _CapturingLogger() + rl = RejectionLogger(log) + + # Same-origin — no warning + rl.maybe_log(None, 'http://localhost:8888', 'localhost:8888') + # Wildcard — no warning + 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') + + assert log.warnings == [] + + +def test_rejection_logger_silent_when_no_origin_header(): + """Non-browser clients (curl, server-to-server) don't send Origin — + they should not trigger the warning.""" + log = _CapturingLogger() + rl = RejectionLogger(log) + + rl.maybe_log(None, None, 'localhost:8888') + rl.maybe_log(None, '', 'localhost:8888') + + assert log.warnings == [] + + +def test_rejection_logger_warning_message_points_user_to_settings(): + """The warning is the ONLY signal users get when their reverse proxy + setup is broken. It must name the origin AND tell them where to fix it.""" + log = _CapturingLogger() + rl = RejectionLogger(log) + + rl.maybe_log(None, 'https://soulsync.example.com', 'internal-host:8888') + + assert len(log.warnings) == 1 + msg = log.warnings[0] + assert 'soulsync.example.com' in msg, "warning must include the rejected origin" + assert 'internal-host:8888' in msg, "warning must include the request Host so users can debug proxy config" + assert 'Settings' in msg, "warning must point users to Settings" + assert 'Allowed' in msg, "warning must name the field they need to edit" + + +def test_rejection_logger_dedup_is_threadsafe(): + """Two threads racing on the same novel origin must result in exactly + one warning, not two. Locks the dedup set internally.""" + log = _CapturingLogger() + rl = RejectionLogger(log) + barrier = threading.Barrier(8) + + def hammer(): + barrier.wait() + for _ in range(50): + rl.maybe_log(None, 'https://race.test', 'localhost:8888') + + threads = [threading.Thread(target=hammer) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert len(log.warnings) == 1 + + +def test_rejection_logger_reset_for_tests_clears_dedup(): + log = _CapturingLogger() + rl = RejectionLogger(log) + + rl.maybe_log(None, 'https://x.com', 'localhost:8888') + assert len(log.warnings) == 1 + + rl.reset_for_tests() + rl.maybe_log(None, 'https://x.com', 'localhost:8888') + assert len(log.warnings) == 2 # logged again after reset + + +# ── log_startup_status ──────────────────────────────────────────────────── + + +def test_startup_status_warns_on_wildcard(): + """The wildcard is a security risk — startup must log a warning that + points users to the settings page, not just an info line.""" + log = _CapturingLogger() + log_startup_status('*', log) + + assert len(log.warnings) == 1 + assert "'*'" in log.warnings[0] + assert 'Settings' in log.warnings[0] + assert log.infos == [] + + +def test_startup_status_info_logs_nonempty_allowlist(): + """Non-empty allow-list → info, so users can confirm their config + actually took effect.""" + log = _CapturingLogger() + log_startup_status(['https://x.com', 'https://y.com'], log) + + assert log.warnings == [] + assert len(log.infos) == 1 + assert 'https://x.com' in log.infos[0] + + +def test_startup_status_silent_on_default_same_origin(): + """None (default) → no log. Same-origin-only is the default; + nothing noteworthy to announce on every startup.""" + log = _CapturingLogger() + log_startup_status(None, log) + + assert log.warnings == [] + assert log.infos == [] diff --git a/web_server.py b/web_server.py index c473e037..77690b3b 100644 --- a/web_server.py +++ b/web_server.py @@ -210,12 +210,35 @@ def _init_flask_secret_key(): app.secret_key = _init_flask_secret_key() # --- WebSocket (Socket.IO) Setup --- -socketio = SocketIO(app, async_mode='threading', cors_allowed_origins='*') +from core.socketio_cors import ( + resolve_cors_origins as _resolve_socketio_cors_origins, + RejectionLogger as _SocketIORejectionLogger, + log_startup_status as _log_socketio_startup_status, +) +_socketio_cors_origins = _resolve_socketio_cors_origins(config_manager) +socketio = SocketIO(app, async_mode='threading', cors_allowed_origins=_socketio_cors_origins) +_log_socketio_startup_status(_socketio_cors_origins, logger) +_socketio_rejection_logger = _SocketIORejectionLogger(logger) # Plex PIN auth requests stored in memory for polling _plex_pin_requests = {} _plex_pin_requests_lock = threading.Lock() +@app.before_request +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`.""" + if not request.path.startswith('/socket.io/'): + return + _socketio_rejection_logger.maybe_log( + _socketio_cors_origins, + request.headers.get('Origin'), + request.headers.get('Host', ''), + request.headers.get('X-Forwarded-Host', ''), + ) + + # --- Profile Context (before_request hook) --- @app.before_request def _set_profile_context(): diff --git a/webui/index.html b/webui/index.html index 9732573f..1bd71056 100644 --- a/webui/index.html +++ b/webui/index.html @@ -5409,6 +5409,14 @@ + +
+ + +
+ Origins (full URL, no trailing slash) allowed to open WebSocket connections to this instance — one per line, or comma-separated. Leave empty for same-origin only (the secure default; works for direct access and most reverse-proxy setups). Add your public domain here if you reach SoulSync via a reverse proxy or custom domain and the WebSocket fails to connect. Use * on its own line to allow any origin (insecure — only do this if you understand why you need it). +
+
diff --git a/webui/static/helper.js b/webui/static/helper.js index 8c6553c1..cae52460 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3441,6 +3441,11 @@ function closeHelperSearch() { // projects that span multiple commits before shipping. Strip the flag at // release time and add a real `date:` line at the top of the version block. const WHATS_NEW = { + '2.4.1': [ + // --- post-2.4.0 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- + { date: 'Unreleased — 2.4.1 dev cycle' }, + { title: 'Lock Down Socket.IO CORS', desc: 'socket.io was accepting websocket connections from any origin (cors=*). now defaults to same-origin only. if your websocket fails after updating, the server logs a clear warning with the rejected origin — add it to settings → security → allowed websocket origins.', page: 'settings' }, + ], '2.4.0': [ // --- April 26, 2026 — Search & Artists unification + reorganize queue --- { date: 'April 26, 2026 — 2.4.0 release' }, diff --git a/webui/static/settings.js b/webui/static/settings.js index 7d10dabf..f2026891 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -996,6 +996,11 @@ async function loadSettingsData() { const requirePin = settings.security?.require_pin_on_launch || false; document.getElementById('security-require-pin').checked = requirePin; + // CORS origins — stored verbatim as the user typed (string). + const corsOrigins = settings.security?.cors_origins || ''; + const corsField = document.getElementById('security-cors-origins'); + if (corsField) corsField.value = corsOrigins; + // Check if admin has a PIN set const profilesRes = await fetch('/api/profiles'); const profilesData = await profilesRes.json(); @@ -2587,6 +2592,7 @@ async function saveSettings(quiet = false) { }, security: { require_pin_on_launch: document.getElementById('security-require-pin')?.checked || false, + cors_origins: document.getElementById('security-cors-origins')?.value?.trim() || '', } };