Socket.IO CORS: polish — match engineio exactly, bound dedup, validate URLs
Self-review pass on the security fix uncovered five issues, all fixed
here:
1. will_reject scheme handling. Engineio compares full {scheme}://{host}
strings, not just hostnames. A TLS-terminating proxy can leave the
backend seeing http while the browser's Origin is https — engineio
rejects, but the original predictor said "allow" → no helpful log
line. Added request_scheme + forwarded_proto params, build full
candidate strings to match engineio.
2. EITHER-forwarded-header rule. 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). The original predictor
only added it when forwarded_host was set — false negative for
misconfigs sending only X-Forwarded-Proto. Now mirrors engineio.
3. will_reject incorrectly rejected missing-Origin requests. Engineio
(server.py:207: `if origin: validate`) skips CORS validation when
no Origin header is sent — non-browser clients (curl etc.) are
intentionally permitted. The original code rejected them. Test was
asserting the wrong behavior. Both fixed.
4. RejectionLogger had unbounded dedup set growth. A hostile actor
opening connections from many distinct fake origins would fill
memory unboundedly. Capped at 100 unique origins (configurable);
when cap hit, one overflow notice is emitted and further rejections
are silently dropped until restart.
5. Lock pattern: the overflow log path called logger.warning() while
holding the dedup lock, inconsistent with the normal path. Fixed
to pick the message under the lock and log after release. Critical
section is now minimal and uniform.
Plus polish:
- Stale module docstring fixed (said "empty list" instead of "None").
- settings.js validates each cors_origins line against a URL regex on
save; toasts a one-shot warning if entries are malformed (resolver
silently filters them, but user gets feedback now).
- web_server.py wiring passes request.scheme + X-Forwarded-Proto so
the predictor has full proxy info.
Tests:
- 51 unit tests in tests/test_socketio_cors.py (was 45). New cases:
* scheme comparison (5 cases including TLS-terminating proxies)
* forwarded_proto-alone misconfig
* missing-origin matches engineio (was asserting wrong behavior)
* dedup cap with overflow + reset
* default cap is reasonable (uses public DEFAULT_DEDUP_CAP constant)
Engineio behavior independently verified by reading engineio/server.py
and engineio/base_server.py source. Predictor mirrors both files.
604 tests pass.
This commit is contained in:
Broque Thomas
parent
013eebf350
commit
0f24739e27
4 changed files with 241 additions and 28 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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 ────────────────────────────────────────────────────
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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', ''),
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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...');
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue