From ed3d12a762525a150202dfd3d4bf107b1097c3a9 Mon Sep 17 00:00:00 2001 From: memosr Date: Fri, 5 Jun 2026 14:51:26 +0300 Subject: [PATCH] fix(security): fail-closed when WebSocket peer is empty in loopback mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- hermes_cli/web_server.py | 12 +++- .../hermes_cli/test_dashboard_auth_ws_auth.py | 56 +++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 3049bb45f99..224e264b8d9 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -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 diff --git a/tests/hermes_cli/test_dashboard_auth_ws_auth.py b/tests/hermes_cli/test_dashboard_auth_ws_auth.py index d4f9dbbdd0c..90969106ad0 100644 --- a/tests/hermes_cli/test_dashboard_auth_ws_auth.py +++ b/tests/hermes_cli/test_dashboard_auth_ws_auth.py @@ -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.