Socket.IO CORS: handle self-review nits
Six items from a Cin-style line-by-line pass on PR #383: - resolve_cors_origins: list of non-string entries (`[None, 123]`) now drops them instead of coercing to junk strings like `'None'`/`'123'`. - will_reject: backwards-compat shim removed. Production callers always pass `request.scheme` (Flask-guaranteed); the shim only existed for tests/non-Flask callers and made the production code path branchier than necessary. Tests now pass scheme explicitly. - maybe_log: redundant `if not origin` early-return dropped. will_reject handles missing origin (engineio's own behavior — server.py:207). - RejectionLogger.__init__: `int(dedup_cap)` wrapped in try/except so bad-type input falls back to DEFAULT_DEDUP_CAP instead of raising. - web_server.py: docstring on the before_request hook explains why the hook fires on every request (Flask doesn't scope before_request to a path prefix; the early-return string compare is the cheapest option). - settings.js: cors-origins URL regex tightened from `[^\s/]+` to `[^\s/?#]+` so query/fragment chars don't pass validation. Engineio would silently fail to match those anyway; better to flag at save. Test changes: - parametrize gained an explicit `scheme` column (12 cases updated). - New explicit case: scheme-mismatch rejects (engineio compares full `{scheme}://{host}` strings). - `test_will_reject_falls_back_to_host_only_when_no_scheme_info` deleted — the shim it tested is gone. - `test_will_reject_honors_x_forwarded_host` now passes scheme info. Net: -9 production lines, -3 test lines. Production code path is straight-line. 603 tests pass.
This commit is contained in:
Broque Thomas
parent
efd2960629
commit
dd4cf130d7
4 changed files with 53 additions and 62 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Reference in a new issue