fix(security): fail-closed when WebSocket peer is empty in loopback mode (#15544) - #50389
Merged
Conversation
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.
Contributor
🔎 Lint report:
|
alt-glitch
added
type/security
6 tasks
This was referenced Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A WebSocket with no identifiable peer (
ws.client is Noneorhost == "") is now rejected on a loopback-bound, auth-disabled dashboard instead of being default-allowed.Root cause: both
_ws_client_reasonand_ws_client_is_allowedreturned "allowed" (None/True) whenclient_hostwas empty. On a loopback bind whose only auth is the legacy?token=query param, an ASGI server behind a misconfigured reverse proxy or a unix-socket transport can deliver an empty/None client, letting an unidentified peer through — a fail-open default-allow.Changes
hermes_cli/web_server.py: emptyclient_hostnow fails closed —_ws_client_reasonreturns amissing_or_empty_peerreason and_ws_client_is_allowedreturnsFalse. Scoped to the loopback path only; the explicit non-loopback--insecurebind and the OAuth-gated path early-return before this check, so public/LAN/gated peers are unaffected.tests/hermes_cli/test_dashboard_auth_ws_auth.py: 5 regression tests covering empty host rejected, None-client rejected, the block reason, and still-allowed under both--insecureand gated modes.Validation
--insecurenon-loopback bindtests/hermes_cli/test_dashboard_auth_ws_auth.pySalvaged from #15544 by @memosr — commit cherry-picked onto current main with authorship preserved.
Infographic