fix(security): fail-closed when WebSocket peer is empty in loopback mode

Per @egilewski's audit on this PR (#15544), the original fix was
correct but the file has refactored since: the four endpoint-local
empty-peer checks have been consolidated into _ws_client_is_allowed
and _ws_client_reason, but the helpers were left fail-open ('no peer
host known means allow' / 'no reason to block').

On a loopback-bound dashboard with auth disabled, an ASGI server
behind a misconfigured proxy or a unix-socket transport can deliver
ws.client == None or ws.client.host == ''. The helpers were treating
that as 'allowed', so the loopback-only peer gate could be bypassed
by anything that suppressed the client tuple in transit. All four
WebSocket endpoints (/api/pty, /api/ws, /api/pub, /api/events) route
through _ws_request_is_allowed -> _ws_client_is_allowed, so the gap
applied uniformly.

Fix:

* _ws_client_is_allowed: return False when client_host is empty
  instead of True. Only reached on loopback bind with auth disabled
  (auth_required=True and explicit non-loopback binds short-circuit
  earlier), so the fail-closed behavior is scoped to the surface
  that needs it.

* _ws_client_reason: return a 'missing_or_empty_peer bound=...'
  block reason instead of None, so the dispatcher's existing
  reason-based rejection path picks it up and the close gets logged
  with a machine-parseable token for diagnosability.

Behavior unchanged for:

* gated mode (auth_required=True) — early-returns True before the
  empty-peer check runs. The OAuth ticket is the auth at that point.
* explicit non-loopback bind (--host 0.0.0.0/::, or a specific LAN
  address, always with --insecure) — early-returns True before the
  empty-peer check runs. DNS-rebinding is still blocked by the
  Host/Origin guard in _ws_host_origin_is_allowed.
* legitimate loopback peers (client_host == '127.0.0.1' / '::1') —
  not affected by the empty-peer branch.

Regression tests added in tests/hermes_cli/test_dashboard_auth_ws_auth.py:

* test_empty_client_host_rejected_in_loopback_mode
* test_missing_client_object_rejected_in_loopback_mode
* test_empty_client_host_reason_is_block

Plus two regression guards to ensure the fix does not over-reach:

* test_empty_client_host_still_allowed_in_insecure_public_mode
* test_empty_client_host_still_allowed_in_gated_mode

All three new fail-closed tests fail without this patch (the helpers
return True / None for an empty peer) and pass with it. The 45
pre-existing tests in test_dashboard_auth_ws_auth.py continue to pass.
This commit is contained in:
memosr 2026-06-05 14:51:26 +03:00 committed by Teknium
parent a4b1554c73
commit ed3d12a762
2 changed files with 66 additions and 2 deletions

View file

@ -11005,7 +11005,12 @@ def _ws_client_reason(ws: "WebSocket") -> Optional[str]:
return None
client_host = ws.client.host if ws.client else ""
if not client_host:
return None
# Fail-closed: a loopback-bound dashboard with auth disabled must
# not accept a WebSocket with no identifiable peer. ASGI servers
# behind a misconfigured proxy or unix socket can deliver
# ws.client == None or "" — treating that as "allowed" would let
# an unidentified peer reach a loopback-only surface.
return f"missing_or_empty_peer bound={bound_host or '?'}"
if client_host in _LOOPBACK_HOSTS:
return None
return f"peer_not_loopback peer={client_host} bound={bound_host or '?'}"
@ -11047,7 +11052,10 @@ def _ws_client_is_allowed(ws: "WebSocket") -> bool:
return True
client_host = ws.client.host if ws.client else ""
if not client_host:
return True
# Fail-closed: see _ws_client_reason for rationale. An empty
# client_host on a loopback-bound dashboard with auth disabled
# must be rejected, not accepted as a default-allow.
return False
return client_host in _LOOPBACK_HOSTS

View file

@ -398,6 +398,62 @@ class TestWsRequestIsAllowedGated:
ws.headers = {"host": "evil.example.com"}
assert web_server._ws_request_is_allowed(ws) is False
# -- security: empty / missing peer must fail closed in loopback mode --
# Regression for the fail-open default-allow where
# ``ws.client is None`` or ``ws.client.host == ""`` was treated as
# "allowed" on a loopback-bound dashboard with auth disabled. ASGI
# servers behind a misconfigured proxy or a unix-socket transport can
# deliver either shape, so both must be rejected explicitly.
def test_empty_client_host_rejected_in_loopback_mode(self, loopback_app):
"""An empty ws.client.host must be rejected on a loopback bind."""
ws = _fake_ws(query={}, client_host="")
ws.headers = {"host": "127.0.0.1:8080"}
assert web_server._ws_client_is_allowed(ws) is False
assert web_server._ws_request_is_allowed(ws) is False
def test_missing_client_object_rejected_in_loopback_mode(self, loopback_app):
"""ws.client is None must be rejected on a loopback bind."""
ws = _fake_ws(query={}, client_host="")
ws.client = None # ASGI servers can omit the client tuple entirely
ws.headers = {"host": "127.0.0.1:8080"}
assert web_server._ws_client_is_allowed(ws) is False
assert web_server._ws_request_is_allowed(ws) is False
def test_empty_client_host_reason_is_block(self, loopback_app):
"""_ws_client_reason must return a block reason for an empty peer,
not ``None`` (which the dispatcher treats as ``allowed``)."""
ws = _fake_ws(query={}, client_host="")
ws.headers = {"host": "127.0.0.1:8080"}
reason = web_server._ws_client_reason(ws)
assert reason is not None
assert "missing_or_empty_peer" in reason
def test_empty_client_host_still_allowed_in_insecure_public_mode(
self, insecure_public_app
):
"""The empty-peer fail-closed guard must only apply to loopback
binds. With an explicit ``--host 0.0.0.0 --insecure`` opt-in, the
loopback-only peer restriction does not run at all, so the empty
peer case bypasses the new guard the same way a legitimate LAN
peer does. Without this, the fix would regress the public-bind
path the dashboard relies on."""
ws = _fake_ws(query={}, client_host="")
ws.headers = {
"host": "192.168.0.222:9120",
"origin": "http://192.168.0.222:9120",
}
assert web_server._ws_client_is_allowed(ws) is True
def test_empty_client_host_still_allowed_in_gated_mode(self, gated_app):
"""The empty-peer fail-closed guard must not apply when the OAuth
gate is active (``auth_required=True``). Gated mode rewrites
``ws.client.host`` via ``proxy_headers=True``, and the ticket is
the auth, so peer-IP is irrelevant on that path."""
ws = _fake_ws(query={}, client_host="")
ws.headers = {"host": "dashboard.example.com"}
assert web_server._ws_client_is_allowed(ws) is True
class TestWsHostOriginGuardOrigins:
"""The WS Origin guard must let the packaged desktop shell connect.