Skip to content

fix: allow dashboard websockets for explicit bound hosts#32357

Closed
pxdsgnco wants to merge 1 commit into
NousResearch:mainfrom
pxdsgnco:fix/dashboard-websocket-bound-host
Closed

fix: allow dashboard websockets for explicit bound hosts#32357
pxdsgnco wants to merge 1 commit into
NousResearch:mainfrom
pxdsgnco:fix/dashboard-websocket-bound-host

Conversation

@pxdsgnco

Copy link
Copy Markdown
Contributor

Summary

  • Allow Dashboard WebSocket peer checks to accept clients matching an explicitly bound non-loopback host/IP.
  • Preserve the stricter loopback-only behaviour for loopback/default dashboard binds.
  • Add regression coverage for explicit tailnet-style bound hosts and loopback rejection.

Why

When the Dashboard is intentionally bound to a tailnet hostname/IP and served through Tailscale Serve, WebSocket upgrades can arrive from the bound non-loopback address. The existing peer check only allowed loopback clients, so /api/events, /api/ws, and /api/pty could disconnect even though Host/Origin validation had already accepted the dashboard boundary.

Test Plan

  • venv/bin/python -m py_compile hermes_cli/web_server.py tests/hermes_cli/test_web_server_host_header.py
  • venv/bin/python -m pytest tests/hermes_cli/test_web_server_host_header.py -q -o 'addopts='
    • Result: 13 passed, 1 warning

Security notes

  • Host/Origin validation remains separate and still runs before accepting the WebSocket upgrade.
  • Non-loopback peers are rejected when the dashboard is bound to loopback or no bound host is configured.
  • Non-loopback peers are only accepted when they match the explicit bound host or its resolved address.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 26, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #20136 (WebSocket proxy peer/origin validation with config-based allowed_hosts). This PR takes a narrower approach: accept WS peers matching the explicitly bound non-loopback host. #20136 adds a full config-based allowlist. Both address the same class of issue (WS rejected behind reverse proxy/Tailscale).

@pxdsgnco

Copy link
Copy Markdown
Contributor Author

Thanks for the pointer — that overlap is real.

This PR was intentionally kept narrow because it came out of a specific production deployment issue with Hermes Dashboard behind Tailscale Serve on a tailnet-only host.

In that setup we saw /api/events, /api/ws, and /api/pty fail their WebSocket upgrades even after the dashboard boundary had already been accepted via Host/Origin validation, because the peer check still only allowed loopback clients.

This change was meant as the smallest fix for that case:

  • keep loopback/default binds strict
  • accept non-loopback WS peers only when they match the explicitly bound host/IP
  • avoid introducing broader proxy configuration surface area

So I agree #20136 is addressing the same class of problem, but with a more general config-driven shape. I’m happy for maintainers to choose whichever direction fits Hermes best.

If the broader trusted_proxy_hosts approach is preferred, I’m also happy to treat this PR as a narrower validated alternative / deployment datapoint rather than pushing both independently.

@teknium1

Copy link
Copy Markdown
Contributor

Landed in PR #35386 (merge commit 234ac00), now on main. Your PR identified the real gap the earlier wildcard fix left open — binding directly to an explicit non-loopback host (Tailscale/LAN IP) still failed the WS handshake. The merged version simplifies the peer-gate logic (it defers to the existing exact-Host guard rather than resolving the bind via getaddrinfo at request time), and you're credited as co-author via the commit trailer. Thanks for the fix and the clear repro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants