fix(dashboard): explain WHY a chat WS connection was refused (#38743)
* Port from google-gemini/gemini-cli#21541: back up corrupted config.yaml When config.yaml fails to parse, load_config() silently falls back to DEFAULT_CONFIG and leaves the broken file on disk. If the user then re-runs the setup wizard or hermes config set (both rewrite config.yaml), their broken-but-recoverable overrides are lost for good. Adapts the policy-file recovery from gemini-cli#21541: on the first parse warning for a given broken file, snapshot it to config.yaml.corrupt.<ts>.bak (best-effort, symlink-guarded, size-deduped) and tell the user where it landed. Unlike Gemini's version we deliberately do NOT reset config.yaml to a clean state — hermes never silently mutates user config, and leaving it means a hand-fixed file is re-read on the next load. Tests: 3 new cases (backup created + content preserved + original untouched; same-size backup dedup; symlink not copied). E2E verified with isolated HERMES_HOME and a real tab-indented broken config. * fix(dashboard): explain WHY a chat WS connection was refused The embedded-chat PTY WebSocket (/api/pty) collapsed every rejection into a bare close code: 4401 for any auth failure, 4403 for three unrelated failures (host mismatch, origin mismatch, peer-IP). Neither the server log nor the browser said which gate fired or why, so a "chat won't connect" report was undiagnosable without a repro. Server (web_server.py): - _ws_auth_reason / _ws_host_origin_reason / _ws_client_reason return a short machine-parseable reason; old bool wrappers kept for callers/tests. - pty_ws splits the overloaded 4403 into 4401 (auth), 4403 (host/origin), 4408 (peer not allowed), 4404 (chat disabled), and sends the reason on the close frame (clamped to the 123-byte RFC6455 limit). - Each path logs one line: 'pty auth rejected reason=.. mode=.. cred=.. peer=..' / 'pty refused: <reason> ..'. Accepted path logs 'pty accepted peer=.. mode=.. cred=..' so an audit shows HOW a peer authed, not just that it did. tui_gateway/ws.py: - 'ws send/write failed' now logs error_type=<ExcName> so an exception whose str() is empty (closed-transport sends) no longer logs 'error='. web/src/pages/ChatPage.tsx: - console.warn the real close code + server reason on every close. - Map 4404/4408 to specific banners; 4401/4403 banners echo the server reason; [session ended] prints the close code. E2E verified all five reject paths + accepted path produce matching close code, wire reason, and server log line.
This commit is contained in:
Teknium
GitHub
parent
c2ca3f01ab
commit
6717914e0a
@ -7014,6 +7014,28 @@ _VALID_CHANNEL_RE = re.compile(r"^[A-Za-z0-9._-]{1,128}$")
|
||||
_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1", "localhost", "testclient"})
|
||||
|
||||
|
||||
def _ws_client_reason(ws: "WebSocket") -> Optional[str]:
|
||||
"""Return a rejection reason for the client IP, or None when allowed.
|
||||
|
||||
Reasons are short machine-parseable tokens logged on the rejection path
|
||||
so a "WS keeps closing" report can be diagnosed from agent.log without a
|
||||
repro. ``None`` means the peer IP passed this gate.
|
||||
|
||||
See :func:`_ws_client_is_allowed` for the full policy rationale.
|
||||
"""
|
||||
if getattr(app.state, "auth_required", False):
|
||||
return None
|
||||
bound_host = (getattr(app.state, "bound_host", "") or "").strip().lower()
|
||||
if bound_host and bound_host not in _LOOPBACK_HOSTS:
|
||||
return None
|
||||
client_host = ws.client.host if ws.client else ""
|
||||
if not client_host:
|
||||
return None
|
||||
if client_host in _LOOPBACK_HOSTS:
|
||||
return None
|
||||
return f"peer_not_loopback peer={client_host} bound={bound_host or '?'}"
|
||||
|
||||
|
||||
def _ws_client_is_allowed(ws: "WebSocket") -> bool:
|
||||
"""Check if the WebSocket client IP is acceptable.
|
||||
|
||||
@ -7054,6 +7076,40 @@ def _ws_client_is_allowed(ws: "WebSocket") -> bool:
|
||||
return client_host in _LOOPBACK_HOSTS
|
||||
|
||||
|
||||
def _ws_host_origin_reason(ws: "WebSocket") -> Optional[str]:
|
||||
"""Return a Host/Origin rejection reason, or None when allowed.
|
||||
|
||||
Mirrors :func:`_ws_host_origin_is_allowed` but yields a short
|
||||
machine-parseable token (``host_mismatch …`` / ``origin_mismatch …``)
|
||||
on rejection so the close path can log *why* the upgrade was refused.
|
||||
"""
|
||||
bound_host = getattr(app.state, "bound_host", None)
|
||||
if not bound_host:
|
||||
return None
|
||||
|
||||
host_header = ws.headers.get("host", "")
|
||||
if not _is_accepted_host(host_header, bound_host):
|
||||
return f"host_mismatch host={host_header or '?'} bound={bound_host}"
|
||||
|
||||
origin = ws.headers.get("origin", "")
|
||||
if not origin:
|
||||
return None
|
||||
|
||||
parsed = urllib.parse.urlparse(origin)
|
||||
if parsed.scheme not in {"http", "https"}:
|
||||
# Non-web origin (packaged Electron: file://, null, app://). The
|
||||
# upstream credential check is the real auth boundary; trust it.
|
||||
# See _ws_host_origin_is_allowed for the full rationale.
|
||||
return None
|
||||
|
||||
if not parsed.netloc:
|
||||
return f"origin_mismatch origin={origin} bound={bound_host}"
|
||||
|
||||
if not _is_accepted_host(parsed.netloc, bound_host):
|
||||
return f"origin_mismatch origin={origin} bound={bound_host}"
|
||||
return None
|
||||
|
||||
|
||||
def _ws_host_origin_is_allowed(ws: "WebSocket") -> bool:
|
||||
"""Apply the dashboard Host/Origin guard to WebSocket upgrades.
|
||||
|
||||
@ -7063,45 +7119,12 @@ def _ws_host_origin_is_allowed(ws: "WebSocket") -> bool:
|
||||
header on WebSocket handshakes; when present, require it to target the
|
||||
same bound dashboard host.
|
||||
"""
|
||||
bound_host = getattr(app.state, "bound_host", None)
|
||||
if not bound_host:
|
||||
return True
|
||||
return _ws_host_origin_reason(ws) is None
|
||||
|
||||
host_header = ws.headers.get("host", "")
|
||||
if not _is_accepted_host(host_header, bound_host):
|
||||
return False
|
||||
|
||||
origin = ws.headers.get("origin", "")
|
||||
if not origin:
|
||||
return True
|
||||
|
||||
parsed = urllib.parse.urlparse(origin)
|
||||
if parsed.scheme not in {"http", "https"}:
|
||||
# Packaged Electron loads the desktop renderer over a non-web origin
|
||||
# such as file://, null, or a custom app:// scheme. This helper is
|
||||
# called only AFTER _ws_auth_ok has already accepted the WS credential,
|
||||
# which is the real auth boundary in every mode:
|
||||
# * loopback bind → legacy dashboard session token
|
||||
# * non-loopback --insecure → legacy session token (Tailscale / LAN)
|
||||
# * OAuth-gated public bind → single-use, 30s-TTL, identity-bound
|
||||
# ?ticket= minted at the cookie-authed POST /api/auth/ws-ticket
|
||||
# A non-web origin can only be produced by a native client (the desktop
|
||||
# shell); a DNS-rebinding attack always arrives from an http(s) origin
|
||||
# and is still match-checked against the bound host below. So once the
|
||||
# credential check upstream has passed, the Origin guard adds nothing
|
||||
# for a non-web origin — trust it in every mode.
|
||||
#
|
||||
# (Earlier revisions restricted this to loopback, then to non-gated
|
||||
# binds; both excluded the packaged desktop talking to a remote
|
||||
# OAuth-gated gateway, whose file:// renderer origin then got rejected
|
||||
# at the WS upgrade even with a valid ticket. The ticket is the gate,
|
||||
# not the origin.)
|
||||
return True
|
||||
|
||||
if not parsed.netloc:
|
||||
return False
|
||||
|
||||
return _is_accepted_host(parsed.netloc, bound_host)
|
||||
def _ws_request_reason(ws: "WebSocket") -> Optional[str]:
|
||||
"""First Host/Origin or peer-IP rejection reason, or None when allowed."""
|
||||
return _ws_host_origin_reason(ws) or _ws_client_reason(ws)
|
||||
|
||||
|
||||
def _ws_request_is_allowed(ws: "WebSocket") -> bool:
|
||||
@ -7109,8 +7132,25 @@ def _ws_request_is_allowed(ws: "WebSocket") -> bool:
|
||||
return _ws_host_origin_is_allowed(ws) and _ws_client_is_allowed(ws)
|
||||
|
||||
|
||||
def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
"""Validate WS-upgrade auth in either loopback or gated mode.
|
||||
def _ws_auth_mode() -> str:
|
||||
"""Short label for the active WS auth mode — logged on every connection."""
|
||||
if getattr(app.state, "auth_required", False):
|
||||
return "gated"
|
||||
bound_host = (getattr(app.state, "bound_host", "") or "").strip().lower()
|
||||
if bound_host and bound_host not in _LOOPBACK_HOSTS:
|
||||
return "insecure"
|
||||
return "loopback"
|
||||
|
||||
|
||||
def _ws_auth_reason(ws: "WebSocket") -> tuple[Optional[str], str]:
|
||||
"""Validate WS-upgrade auth; return ``(reason, credential)``.
|
||||
|
||||
``reason`` is None when the credential is accepted, else a short
|
||||
machine-parseable token explaining the rejection (``no_credential``,
|
||||
``token_mismatch``, ``ticket_invalid``, ``internal_invalid``).
|
||||
``credential`` names which credential type was presented (``ticket``,
|
||||
``internal``, ``token``, or ``none``) so the accepted path can log *how*
|
||||
a peer authed, not just that it did.
|
||||
|
||||
Loopback / ``--insecure``: legacy ``?token=<_SESSION_TOKEN>`` query
|
||||
parameter, constant-time compared.
|
||||
@ -7131,9 +7171,8 @@ def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
(the SPA bundle isn't carrying the token any longer, and a leaked
|
||||
``_SESSION_TOKEN`` must not grant WS access once the gate is engaged).
|
||||
|
||||
Returns True if the WS should be accepted; callers close with the
|
||||
appropriate WS code (4401) on False. Audit-logs the rejection so
|
||||
operators can debug "WS keeps closing" issues from the log.
|
||||
Audit-logs the rejection so operators can debug "WS keeps closing"
|
||||
issues from the log.
|
||||
"""
|
||||
auth_required = bool(getattr(app.state, "auth_required", False))
|
||||
if auth_required:
|
||||
@ -7153,7 +7192,7 @@ def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
if internal:
|
||||
try:
|
||||
consume_internal_credential(internal)
|
||||
return True
|
||||
return None, "internal"
|
||||
except TicketInvalid as exc:
|
||||
audit_log(
|
||||
AuditEvent.WS_TICKET_REJECTED,
|
||||
@ -7161,15 +7200,15 @@ def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
ip=(ws.client.host if ws.client else ""),
|
||||
path=ws.url.path,
|
||||
)
|
||||
return False
|
||||
return "internal_invalid", "internal"
|
||||
|
||||
ticket = ws.query_params.get("ticket", "")
|
||||
if not ticket:
|
||||
return False
|
||||
return "no_credential", "none"
|
||||
|
||||
try:
|
||||
consume_ticket(ticket)
|
||||
return True
|
||||
return None, "ticket"
|
||||
except TicketInvalid as exc:
|
||||
audit_log(
|
||||
AuditEvent.WS_TICKET_REJECTED,
|
||||
@ -7177,10 +7216,19 @@ def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
ip=(ws.client.host if ws.client else ""),
|
||||
path=ws.url.path,
|
||||
)
|
||||
return False
|
||||
return "ticket_invalid", "ticket"
|
||||
|
||||
token = ws.query_params.get("token", "")
|
||||
return hmac.compare_digest(token.encode(), _SESSION_TOKEN.encode())
|
||||
if not token:
|
||||
return "no_credential", "none"
|
||||
if hmac.compare_digest(token.encode(), _SESSION_TOKEN.encode()):
|
||||
return None, "token"
|
||||
return "token_mismatch", "token"
|
||||
|
||||
|
||||
def _ws_auth_ok(ws: "WebSocket") -> bool:
|
||||
"""True when the WS-upgrade credential is accepted. See _ws_auth_reason."""
|
||||
return _ws_auth_reason(ws)[0] is None
|
||||
|
||||
# Per-channel subscriber registry used by /api/pub (PTY-side gateway → dashboard)
|
||||
# and /api/events (dashboard → browser sidebar). Keyed by an opaque channel id
|
||||
@ -7332,22 +7380,58 @@ def _channel_or_close_code(ws: WebSocket) -> Optional[str]:
|
||||
return channel if _VALID_CHANNEL_RE.match(channel) else None
|
||||
|
||||
|
||||
def _ws_close_reason(text: str) -> str:
|
||||
"""Clamp a WS close reason to the protocol's 123-byte UTF-8 limit.
|
||||
|
||||
RFC 6455 caps the close-frame reason at 123 bytes; uvicorn raises if a
|
||||
longer string is passed. Our reasons embed an attacker-controlled origin,
|
||||
so truncate defensively rather than crash the close handler.
|
||||
"""
|
||||
encoded = text.encode("utf-8", "replace")
|
||||
if len(encoded) <= 123:
|
||||
return text
|
||||
return encoded[:120].decode("utf-8", "ignore") + "..."
|
||||
|
||||
|
||||
@app.websocket("/api/pty")
|
||||
async def pty_ws(ws: WebSocket) -> None:
|
||||
peer = ws.client.host if ws.client else "?"
|
||||
|
||||
if not _DASHBOARD_EMBEDDED_CHAT_ENABLED:
|
||||
await ws.close(code=4403)
|
||||
_log.info("pty refused: embedded chat disabled peer=%s", peer)
|
||||
await ws.close(code=4404, reason="embedded chat disabled")
|
||||
return
|
||||
|
||||
# --- auth + loopback check (before accept so we can close cleanly) ---
|
||||
if not _ws_auth_ok(ws):
|
||||
await ws.close(code=4401)
|
||||
# --- auth + host/origin/peer check (before accept so we can close
|
||||
# cleanly AND tell the client WHY via the close code + reason).
|
||||
# Each gate maps to a distinct close code so the log and the
|
||||
# browser banner agree on the cause:
|
||||
# 4401 bad credential 4403 host/origin mismatch
|
||||
# 4408 peer not allowed 4404 chat disabled
|
||||
auth_reason, cred = _ws_auth_reason(ws)
|
||||
mode = _ws_auth_mode()
|
||||
if auth_reason is not None:
|
||||
_log.warning(
|
||||
"pty auth rejected reason=%s mode=%s cred=%s peer=%s",
|
||||
auth_reason, mode, cred, peer,
|
||||
)
|
||||
await ws.close(code=4401, reason=_ws_close_reason(f"auth: {auth_reason}"))
|
||||
return
|
||||
|
||||
if not _ws_request_is_allowed(ws):
|
||||
await ws.close(code=4403)
|
||||
host_origin_reason = _ws_host_origin_reason(ws)
|
||||
if host_origin_reason is not None:
|
||||
_log.warning("pty refused: %s peer=%s", host_origin_reason, peer)
|
||||
await ws.close(code=4403, reason=_ws_close_reason(host_origin_reason))
|
||||
return
|
||||
|
||||
client_reason = _ws_client_reason(ws)
|
||||
if client_reason is not None:
|
||||
_log.warning("pty refused: %s", client_reason)
|
||||
await ws.close(code=4408, reason=_ws_close_reason(client_reason))
|
||||
return
|
||||
|
||||
await ws.accept()
|
||||
_log.info("pty accepted peer=%s mode=%s cred=%s", peer, mode, cred)
|
||||
|
||||
# On native Windows, the POSIX PTY bridge can't be imported. Tell the
|
||||
# client and close cleanly rather than pretending the feature works.
|
||||
|
||||
@ -100,7 +100,10 @@ class WSTransport:
|
||||
return not self._closed
|
||||
except Exception as exc:
|
||||
self._closed = True
|
||||
_log.warning("ws write failed peer=%s error=%s", self._peer, exc)
|
||||
_log.warning(
|
||||
"ws write failed peer=%s error_type=%s error=%s",
|
||||
self._peer, type(exc).__name__, exc,
|
||||
)
|
||||
return False
|
||||
|
||||
async def write_async(self, obj: dict) -> bool:
|
||||
@ -115,7 +118,10 @@ class WSTransport:
|
||||
await self._ws.send_text(line)
|
||||
except Exception as exc:
|
||||
self._closed = True
|
||||
_log.warning("ws send failed peer=%s error=%s", self._peer, exc)
|
||||
_log.warning(
|
||||
"ws send failed peer=%s error_type=%s error=%s",
|
||||
self._peer, type(exc).__name__, exc,
|
||||
)
|
||||
|
||||
def close(self) -> None:
|
||||
self._closed = True
|
||||
|
||||
@ -603,19 +603,50 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) {
|
||||
if (unmounting) {
|
||||
return;
|
||||
}
|
||||
// Surface the real cause to the browser console on every close so a
|
||||
// "chat won't connect" report can be diagnosed without server access.
|
||||
// The server sends a machine-parseable reason on every rejection (see
|
||||
// pty_ws in web_server.py); echo it verbatim alongside the close code.
|
||||
const why = ev.reason ? ` reason=${ev.reason}` : "";
|
||||
console.warn(`[chat] PTY WebSocket closed code=${ev.code}${why}`);
|
||||
if (ev.code === 4401) {
|
||||
setBanner("Auth failed. Reload the page to refresh the session token.");
|
||||
setBanner(
|
||||
ev.reason
|
||||
? `Auth failed (${ev.reason}). Reload to refresh the session.`
|
||||
: "Auth failed. Reload the page to refresh the session token.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (ev.code === 4403) {
|
||||
setBanner("Chat is only reachable from localhost.");
|
||||
// Host/Origin mismatch (DNS-rebinding guard).
|
||||
setBanner(
|
||||
ev.reason
|
||||
? `Refused: ${ev.reason}.`
|
||||
: "Refused: request host/origin doesn't match the dashboard.",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (ev.code === 4404) {
|
||||
setBanner(
|
||||
"Embedded chat is disabled on this server (start it with --tui).",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (ev.code === 4408) {
|
||||
setBanner(
|
||||
ev.reason
|
||||
? `Refused: ${ev.reason}.`
|
||||
: "Refused: your client isn't permitted (server bound to localhost only).",
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (ev.code === 1011) {
|
||||
// Server already wrote an ANSI error frame.
|
||||
return;
|
||||
}
|
||||
term.write("\r\n\x1b[90m[session ended]\x1b[0m\r\n");
|
||||
term.write(
|
||||
`\r\n\x1b[90m[session ended (code ${ev.code})]\x1b[0m\r\n`,
|
||||
);
|
||||
};
|
||||
|
||||
// Keystrokes → PTY.
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user