fix(web_server): propagate --insecure flag to WebSocket security guards#31779
fix(web_server): propagate --insecure flag to WebSocket security guards#31779pranvgarg wants to merge 2 commits into
Conversation
start_server() accepts allow_public (the value of --insecure) and uses it to skip the startup SystemExit guard, but never writes it to app.state. As a result, every getattr(app.state, "insecure", False) call in the runtime security guards always evaluates to False — making the flag a no-op at request time. The practical effect: when the dashboard is bound to a non-loopback address via --insecure, _ws_client_is_allowed() still enforces the loopback-only IP check and rejects every WebSocket upgrade with 403. HTTP endpoints are unaffected because _is_accepted_host() resolves the bound host match independently of app.state.insecure. Fix: - Write app.state.insecure = allow_public in start_server() so the flag is visible to all request handlers at runtime. - Short-circuit _ws_client_is_allowed() when insecure mode is active. The operator has already accepted the security trade-off by passing --insecure; enforcing the loopback IP restriction on top of that is inconsistent with the intent of the flag. Tests added to TestInsecureMode: - Baseline: non-loopback client is blocked without --insecure. - Non-loopback client is accepted when --insecure is active. - start_server() correctly persists allow_public to app.state.insecure.
The original TestInsecureMode.test_start_server_writes_insecure_to_app_state patched "hermes_cli.web_server.uvicorn.run", which fails because uvicorn is imported lazily inside start_server() — there is no module-level attribute named uvicorn on hermes_cli.web_server. Patch uvicorn.run on the uvicorn module directly; the test assertions are unchanged. Caught when rebasing onto v0.15.1 and running the test suite locally.
20eb675 to
a6443c7
Compare
|
Force-pushed a rebase onto current Relationship to other open PRsThis overlaps with #33278, #32614, and #25072 — all targeting the
#33278 / #32614 short-circuit on This PR instead writes User-visible symptom is tracked in #33265 (Chat WebSocket rejected, Happy to close this in favor of any consolidating fix the maintainers prefer, or to have it superseded if a broader PR (e.g. #25072) lands first. Otherwise, would appreciate workflow approval to verify CI on the rebase. |
|
Superseded by PR #35141 (commit e8076c1), already on Closing as a duplicate. Thanks for the fix and the thorough host-header test coverage. |
Problem
When the dashboard is bound to a non-loopback address using
--insecure, all WebSocket connections to/api/pty,/api/ws,/api/pub, and/api/eventsfail with 403 Forbidden. HTTP endpoints work fine.Root cause
start_server()acceptsallow_public(the runtime value of--insecure) and uses it to skip the startupSystemExitguard — but never writes it toapp.state:Every
getattr(app.state, "insecure", False)call in the runtime security guards therefore always returnsFalse, making--insecurea no-op at request time.The direct consequence:
_ws_client_is_allowed()enforces loopback-only peer IPs unconditionally. When the server is bound to a non-loopback address, the connecting client also has a non-loopback IP, so every WebSocket upgrade is rejected. HTTP works because_is_accepted_host()resolves the host match fromapp.state.bound_hostdirectly, which is correctly set.Fix
app.state.insecure = allow_publicinstart_server()._ws_client_is_allowed()whenapp.state.insecureisTrue. The operator has already accepted the security trade-off by passing--insecure; enforcing the loopback IP gate on top of that contradicts the intent of the flag.Tests
Three new cases in
TestInsecureMode:test_non_loopback_client_rejected_without_insecuretest_non_loopback_client_allowed_with_insecure--insecureis activetest_start_server_writes_insecure_to_app_statestart_server()correctly persistsallow_publictoapp.state