fix(web_server): accept dashboard.public_url as a valid WebSocket Origin when loopback-bound - #17
fix(web_server): accept dashboard.public_url as a valid WebSocket Origin when loopback-bound#17bbasketballer75 wants to merge 3 commits into
Conversation
…ndows The TUI dependency npm-install subprocess.run() call had no creationflags at all, unlike every other Windows-facing subprocess call in this codebase (which use windows_hide_flags() for exactly this). On a system where Windows Terminal is set as the default terminal-delegation handler, an unflagged console-subsystem child (npm.cmd) gets its own new, visible console -- even when spawned from an already-windowless pythonw.exe parent (e.g. a Windows Scheduled Task). Confirmed empirically on a live install: a Windows Terminal window appeared in lockstep with every dashboard restart that triggered this install path, and disappeared entirely once windows_hide_flags() was added. Adds a regression test in test_windows_subprocess_no_window_flags.py (the existing home for this exact contract across the codebase), verified to fail against the pre-fix code and pass against the fix.
…oudflared+Access) (local WIP)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR updates the dashboard WebSocket Host/Origin validation to support loopback-bound deployments that are accessed via a reverse proxy/tunnel (notably cloudflared with Cloudflare Access) by allowing dashboard.public_url to be treated as an acceptable Origin/Host in that topology. It also adds Windows-specific subprocess creation flags to prevent visible console windows when spawning npm for TUI dependency/build steps, plus a regression test for that behavior.
Changes:
- Accept
dashboard.public_urlas a valid WebSocket Origin/Host when the server is bound to loopback. - Add
windows_hide_flags()-backedcreationflagsto the TUInpmsubprocess calls on Windows. - Add a test ensuring the TUI
npm installspawn includesCREATE_NO_WINDOW.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hermes_cli/web_server.py |
Extends WebSocket Host/Origin acceptance logic for loopback-bound + public URL deployments. |
hermes_cli/main.py |
Adds Windows hidden-console creationflags to npm spawns for TUI install/build. |
tests/test_windows_subprocess_no_window_flags.py |
Adds coverage to ensure the TUI dependency install spawn includes hidden-console flags on Windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| encoding="utf-8", | ||
| errors="replace", | ||
| creationflags=windows_hide_flags(), | ||
| ) |
| public_host: str = "" | ||
| if bound_host in _LOOPBACK_HOSTS: | ||
| try: | ||
| from hermes_cli.dashboard_auth.prefix import resolve_public_url | ||
| import urllib.parse as _up | ||
| _purl = resolve_public_url() | ||
| if _purl: | ||
| public_host = (_up.urlparse(_purl).netloc or "").lower() | ||
| except Exception: # noqa: BLE001 — best-effort; never fail-closed on config lookup | ||
| public_host = "" | ||
|
|
||
| def _host_accepted(value: str) -> bool: | ||
| if _is_accepted_host(value, bound_host): | ||
| return True | ||
| if public_host and value and value.split(":", 1)[0].lower() == public_host: | ||
| return True | ||
| return False |
|
Closing as redundant with upstream PR NousResearch#65965. Same dashboard Verified by diffing this PR's substantive change against the upstream PR (ignoring the co-bundled 🤖 Closed by Claude Code during a repo cleanup audit. |
…pshot shape pin Three CI-red follow-ups on the /save rework: - cli.py save_conversation: getattr-guard _session_db/session_id so SimpleNamespace/object.__new__ test doubles (and any embedder passing a minimal stub) don't AttributeError (pitfall #17 pattern). - gateway/slash_commands.py: route through the awaited async_session_store.get_or_create_session boundary — the architecture test forbids raw session_store calls in async gateway code. - tests/cli/test_save_conversation_location.py: /save now emits the canonical export_session payload shape; the session id key is "id" (was "session_id" in the legacy snapshot format) — update the pin. The slice-8 lost-and-found failure was pre-existing on main and is fixed there by f2a30fa (test: derive lost-and-found synthetic width from the live schema); picked up via rebase.
What this PR does
Headline fix in
hermes_cli/web_server.py(+27 −2): accept theHostheader andOriginheader matchingdashboard.public_urlwhen loopback-bound, so the dashboard WebSocket works throughcloudflaredtunnels with Cloudflare Access in front of it.Why
When
dashboard.public_urlis configured (ahttps://*.trycloudflare.comURL), the dashboard SPA on the remote side initiates awss://connection. The Python WS server was origin-restricted tohttp://127.0.0.1andhttp://localhost, soOrigin: https://…trycloudflare.comwas rejected — the WebSocket handshake silently refused, the dashboard stayed disconnected, and the user saw "offline". This branch adds the matching rule.Diff
Verification
Local repro: started the dashboard with
dashboard.public_url=https://<random-slug>.trycloudflare.comconfigured inconfig.yaml, opened the dashboard from the upstream URL, confirmed the WebSocket connects and the data starts streaming. Without this branch: handshake fails with a 403-style rejection; with it: status flickers to online within ~2 seconds.Co-bundled tests
Each Tier-2 branch in this set carries a co-bundle of
hermes_cli/main.py(+13) andtests/test_windows_subprocess_no_window_flags.py(+37) — the windows_hide_flags addition on the TUI npm subprocess, plus its test scaffold. All five Tier-2 branches descend fromlocal/tui-install-windows-console-fix, so each inherits that parent. The windows_hide_flags addition is on the same Windows-subprocess family as these WIP changes; it is appropriate supporting infrastructure. If maintainers prefer a rebase-direct-to-mainfor each, happy to push rebased PRs — say the word.Related work
PR NousResearch#65965 (already open upstream for the same fix on
local/ws-origin-fix). This is bbasketballer75's local WIP variant — same intent, scoped for the fork's review queue so the fix doesn't conflict on its own PR lane.