fix(dashboard): accept dashboard.public_url as a valid WebSocket Origin when loopback-bound - #65965
fix(dashboard): accept dashboard.public_url as a valid WebSocket Origin when loopback-bound#65965bbasketballer75 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the dashboard WebSocket Host/Origin guard to support a common “loopback bind + reverse proxy” deployment by allowing the operator-configured dashboard.public_url host as an additional accepted Host/Origin when the server is bound to loopback.
Changes:
- Adds best-effort resolution of
dashboard.public_url(viaresolve_public_url()) inside_ws_host_origin_reason()when loopback-bound. - Extends Host/Origin acceptance logic to allow either the bound host or the resolved public host (loopback-only and opt-in via config).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tonydwb
left a comment
There was a problem hiding this comment.
Looks good. No obvious issues found.
Reviewed by Hermes Agent
|
Thanks for the focused reverse-proxy fix. The premise remains present on current The final diff is appropriately narrow: it is loopback-gated, falls back to the existing guard when no valid public URL resolves, and reuses Automated hermes-sweeper review. |
|
Additional local validation against current
The normal HTTP host middleware remains stricter and rejects that public Host when the dashboard is loopback-bound. The public host is operator-configured, so this is not a claim of an arbitrary-host bypass; it is a concrete expansion and HTTP/WS policy mismatch beyond the narrow reverse-proxy Origin fix. The focused dashboard/host/WS suites passed ( A narrower contract would keep |
|
Thanks @Kinkoolino-Hermes for the local validation pass. The Host boundary widening is a fair catch, and it's outside the original scope of this PR. The intent of the change was strictly the Origin header (the browser's Cross-Origin WebSocket gate). Widening Host at the same time is a separate concern that should land as its own PR with a justification for each additional accepted value, OR this PR should be narrowed to ONLY accept dashboard.public_url as a valid Origin (not Host). Going with the narrower approach: will push a follow-up commit that removes the Host-widening portion of this PR. The loopback-only Host gate stays untouched, and only the Origin check is relaxed to accept dashboard.public_url when loopback-bound. If reviewers want the Host widening as a separate change, that can land as PR #2 with its own discussion. Will post the commit once it's ready. |
f2521af to
9fd33ea
Compare
|
Follow-up per @Kinkoolino-Hermes' local validation pass — pushed a narrowing commit (9fd33ead2) that reverts the Host header widening. Before this commit: After this commit:
The comment in the code explains the rationale (browsers send Origin across reverse proxies like cloudflared, but Host is rewritten upstream to localhost in those topologies, so relaxing it would broaden the surface unnecessarily).
PR diff is now +129/-1 on 2 files (was +125/-2). The 4-line increase is the explanatory comment. The 1-line net deletion is reverting one |
Fixes dashboard WebSocket handshake rejecting the Cloudflare-tunneled Origin header (1006 close) when dashboard.public_url is set. Adds _host_accepted() honoring resolve_public_url() alongside the bound loopback host.
Copilot review feedback on NousResearch#65965: - public_host was derived via urlparse(...).netloc, which keeps any port, while the comparison stripped the port from the incoming value first — a public_url with an explicit port could never match. - The same split(':', 1)[0] also broke on IPv6 literals. Fix: derive public_host via .hostname (strips port, unwraps IPv6 brackets) and reuse _is_accepted_host for the comparison instead of re-implementing port/bracket handling ad hoc. Adds 3 regression tests, verified to fail against the pre-fix code and pass against the fix.
- hermes_cli/web_server.py: urllib.parse is already imported at module level; drop the redundant local 'import urllib.parse as _up' alias and use the module-level import directly, matching the rest of the function. - tests/hermes_cli/test_web_server_host_header.py: drop raising=False on the resolve_public_url monkeypatch. It's a real, existing function (unlike the dynamic FastAPI app.state attributes patched elsewhere in this file, where raising=False is correct) -- raising=False here could silently mask the symbol being renamed/removed, letting the test keep 'passing' against a mock that no longer matches production code. Addresses Copilot review feedback on PR NousResearch#66076, where this code incorrectly also appeared due to a branch-history mistake (see that PR's other review comment) -- fixing it here, in the PR where this code actually belongs.
Reverts the Host header widening introduced in 2c3d9b3bd per @Kinkoolino-Hermes' review on PR NousResearch#65965. The original intent was to let browsers send Origin across reverse proxies (cloudflared + Cloudflare Access) where Host is rewritten to localhost upstream. Widening the Host check too is a separate concern that should land as its own PR with its own justification — not bundled here. The Host header check now stays gated to the bound loopback host. The Origin header check still accepts dashboard.public_url when loopback-bound (the behavior the PR was originally trying to ship). The change is local to _ws_host_origin_reason(); _host_accepted() is preserved for the Origin branch and can be moved/reused by the future Host-widening PR.
9fd33ea to
bc49299
Compare
|
Rebased onto current This is the same narrowing fix @Kinkoolino-Hermes' local-validation pass flagged Host-header widening on. Base in the previous reply ( Re-review welcome — particularly on whether the narrowing still satisfies the original "public_url as a valid WebSocket Origin" requirement. |
…eview Per @Kinkoolino-Hermes's review: the public_url relaxation applies to Origin only, Host stays gated to bound_host unchanged. These two cases lock that in -- Host=public_url-host must still be rejected whether Origin is the ordinary bound-loopback value or the public_url host too, proving rejection is driven by the unwidened Host check specifically, not by Origin happening to also mismatch.
|
@Kinkoolino-Hermes — added the two rejection cases you asked for: |
Duplicate of #62639 — both patches accept the configured dashboard.public_url Origin for a loopback-bound dashboard while retaining the Host-header guard. |
Correction: related to #62639, not a duplicate. Both permit a configured public dashboard origin behind a loopback proxy, but current #65965 matches the configured host while #62639 requires an exact scheme-and-netloc origin policy. A maintainer should choose the contract. |
fix(dashboard): accept
dashboard.public_urlas a valid WebSocket Origin/Host when loopback-boundSummary
The dashboard's WebSocket Origin/Host guard (
_ws_host_origin_reasoninhermes_cli/web_server.py) currently rejects any WebSocket upgrade whoseHostheader orOriginheader doesn't match the bound dashboard host. This breaks the loopback-bind + reverse-proxy topology that production users rely on — most notably the cloudflared + Cloudflare Access setup, where:127.0.0.1:9720(loopback-only, no public attack surface).cloudflaredterminates TLS at the edge and proxies HTTP/HTTPS into the local service, withoriginRequest.httpHostHeader: localhostrewriting theHostheader tolocalhost.https://<public.host>/chatand then upgrades towss://<public.host>/api/ws?....cloudflareddoes not rewrite theOriginheader. Browsers setOrigin: https://<public.host>based on the document origin, and that header reaches the dashboard unchanged.Origin: https://<public.host>against the bound host (localhost) and rejects the upgrade with403 Forbidden. The browser sees the failed upgrade asWebSocket close code 1006 (abnormal closure), and the chat surface appears dead even though the user is fully authenticated.The dashboard already has a
dashboard.public_urlconfig option (used for OAuthredirect_uriandX-Forwarded-Prefixreconstruction). This PR extends its semantics so that, when the dashboard is loopback-bound ANDdashboard.public_urlis configured, the Host/Origin guard accepts the public URL's netloc in addition to the bound host. Operators opt into this explicitly — emptypublic_urlkeeps the old strict-loopback behavior.Reproduction (before this PR)
hermes dashboard --host 127.0.0.1 --port 9720service: http://localhost:9720+httpHostHeader: localhostdashboard.public_url: https://your.hosthttps://your.host/chat, log in via Cloudflare Access/api/auth/meworks when the SPA injects the session token, but every WS upgrade gets403from the Origin guard.Fix
When
bound_host in _LOOPBACK_HOSTSanddashboard.public_urlresolves to a valid URL, the Host/Origin guard also accepts requests whoseHostorOriginnetloc matchespublic_url's host (after stripping any port). This is gated on both conditions — the existing strict-loopback behavior is unchanged whenpublic_urlis empty or when the dashboard is bound to a non-loopback host.The patched helper:
…replaces the two existing
_is_accepted_host(...)call sites in_ws_host_origin_reason.public_hostis derived fromresolve_public_url()(existing helper inhermes_cli.dashboard_auth.prefix); the lookup is best-effort and falls through to the strict-loopback check if config loading fails for any reason.Why this is safe
dashboard.public_urlis non-empty. Existing loopback-bind users withoutpublic_urlsee no difference.localhost/loopback aliases for--insecurebinds via_is_accepted_host).if bound_host in _LOOPBACK_HOSTSguard means a--host 0.0.0.0or Tailscale-IP bind with apublic_urlset still runs the original guard. The--insecureflag's explicit-opt-in semantics are preserved.Origin: https://evil.example.comreturns403even whenpublic_urlis set; only the operator-declared public host passes.Test matrix (verified locally on Windows, dashboard at
127.0.0.1:9720,public_url=https://hermes.theporadas.com)127.0.0.1:9720localhosthttps://hermes.theporadas.com127.0.0.1:9720https://evil.example.comevil.example.comhttps://hermes.theporadas.comDiff
Operator-facing changes
None. This is purely additive — operators who don't set
dashboard.public_urlsee no change. The existingdashboard.public_urloption (already used by OAuth redirect_uri handling andX-Forwarded-Prefixresolution) gains one more consumer.Recommended config for cloudflared-fronted installs:
Plus the existing cloudflared
config.ymlshape:Local re-apply after
hermes updateUntil this lands, the patch file at
~/.hermes/patches/ws-public-url.patch(or equivalent) can be re-applied with:Related context
dashboard.public_urlmechanism was introduced bya890389b6 feat(dashboard-auth): HERMES_DASHBOARD_PUBLIC_URL / dashboard.public_url override._ws_client_reason(line ~14536) for empty peer IPs is unrelated and unaffected.--insecureflag's bypass path (bound_host not in _LOOPBACK_HOSTS) is unchanged.