fix(desktop): address Copilot review on xAI loopback flow
- web_server: join the callback-server thread in the start error path so a failed discovery/URL build doesn't leave a daemon thread running - web_server: loopback worker now bails if the session was cancelled while waiting for the callback or exchanging the code, instead of persisting tokens the user no longer wants (+ regression test) - onboarding: fall back to window.open when the desktop bridge's openExternal is unavailable, so the flow never silently stalls
This commit is contained in:
@ -411,6 +411,18 @@ export async function refreshOnboarding(ctx: OnboardingContext) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Open a sign-in URL via the desktop bridge, falling back to window.open
|
||||
// when the bridge isn't present (e.g. the web dashboard / dev preview) so
|
||||
// the flow never silently stalls in a waiting state. Mirrors the pattern in
|
||||
// apps/desktop/src/app/artifacts/index.tsx.
|
||||
async function openSignInUrl(url: string) {
|
||||
if (window.hermesDesktop?.openExternal) {
|
||||
await window.hermesDesktop.openExternal(url)
|
||||
} else {
|
||||
window.open(url, '_blank', 'noopener,noreferrer')
|
||||
}
|
||||
}
|
||||
|
||||
export async function startProviderOAuth(provider: OAuthProvider, ctx: OnboardingContext) {
|
||||
clearPoll()
|
||||
|
||||
@ -425,7 +437,7 @@ export async function startProviderOAuth(provider: OAuthProvider, ctx: Onboardin
|
||||
try {
|
||||
const start = await startOAuthLogin(provider.id)
|
||||
const browserUrl = start.flow === 'device_code' ? start.verification_url : start.auth_url
|
||||
await window.hermesDesktop?.openExternal(browserUrl)
|
||||
await openSignInUrl(browserUrl)
|
||||
|
||||
if (start.flow === 'pkce') {
|
||||
setFlow({ status: 'awaiting_user', provider, start, code: '' })
|
||||
|
||||
@ -3540,12 +3540,17 @@ def _start_xai_loopback_flow() -> Dict[str, Any]:
|
||||
)
|
||||
except Exception:
|
||||
# Binding succeeded but URL construction failed — release the socket
|
||||
# so we don't leak a listener on the loopback port.
|
||||
# and join the serving thread so we don't leak a listener (or a
|
||||
# lingering daemon thread) on the loopback port.
|
||||
try:
|
||||
server.shutdown()
|
||||
server.server_close()
|
||||
except Exception:
|
||||
pass
|
||||
try:
|
||||
thread.join(timeout=1.0)
|
||||
except Exception:
|
||||
pass
|
||||
raise
|
||||
|
||||
sid, sess = _new_oauth_session("xai-oauth", "loopback")
|
||||
@ -3589,6 +3594,14 @@ def _xai_loopback_worker(session_id: str) -> None:
|
||||
s["status"] = "error"
|
||||
s["error_message"] = message
|
||||
|
||||
def _cancelled() -> bool:
|
||||
# The session is removed from the registry when the user cancels
|
||||
# (DELETE /sessions/{id}). If that happened while we were blocked on
|
||||
# the callback or token exchange, abort instead of persisting tokens
|
||||
# the user no longer wants.
|
||||
with _oauth_sessions_lock:
|
||||
return session_id not in _oauth_sessions
|
||||
|
||||
try:
|
||||
callback = hauth._xai_wait_for_callback(
|
||||
sess["server"],
|
||||
@ -3600,6 +3613,9 @@ def _xai_loopback_worker(session_id: str) -> None:
|
||||
_fail(f"xAI authorization timed out: {exc}")
|
||||
return
|
||||
|
||||
if _cancelled():
|
||||
return
|
||||
|
||||
if callback.get("error"):
|
||||
detail = callback.get("error_description") or callback["error"]
|
||||
_fail(f"xAI authorization failed: {detail}")
|
||||
@ -3638,6 +3654,8 @@ def _xai_loopback_worker(session_id: str) -> None:
|
||||
"expires_in": payload.get("expires_in"),
|
||||
"token_type": str(payload.get("token_type") or "Bearer").strip() or "Bearer",
|
||||
}
|
||||
if _cancelled():
|
||||
return
|
||||
hauth._save_xai_oauth_tokens(
|
||||
tokens,
|
||||
discovery=sess.get("discovery"),
|
||||
|
||||
@ -489,6 +489,50 @@ def test_xai_loopback_worker_fails_on_state_mismatch(monkeypatch):
|
||||
ws._oauth_sessions.pop(session_id, None)
|
||||
|
||||
|
||||
def test_xai_loopback_worker_skips_persist_when_cancelled(monkeypatch):
|
||||
"""If the session is cancelled while waiting, the worker must not persist."""
|
||||
from hermes_cli import auth as auth_mod
|
||||
from hermes_cli import web_server as ws
|
||||
|
||||
session_id = "xai-loopback-cancel-test"
|
||||
ws._oauth_sessions[session_id] = {
|
||||
"session_id": session_id,
|
||||
"provider": "xai-oauth",
|
||||
"flow": "loopback",
|
||||
"created_at": time.time(),
|
||||
"status": "pending",
|
||||
"error_message": None,
|
||||
"server": object(),
|
||||
"thread": object(),
|
||||
"callback_result": {},
|
||||
"redirect_uri": "http://127.0.0.1:56121/callback",
|
||||
"verifier": "verifier",
|
||||
"challenge": "challenge",
|
||||
"state": "st",
|
||||
"token_endpoint": "https://auth.x.ai/oauth2/token",
|
||||
"discovery": {},
|
||||
}
|
||||
|
||||
def _wait_then_cancel(*args, **kwargs):
|
||||
# Simulate the user cancelling (DELETE /sessions/{id}) while we were
|
||||
# blocked on the callback: the session vanishes, then a valid code
|
||||
# arrives. The worker must notice and bail before persisting.
|
||||
ws._oauth_sessions.pop(session_id, None)
|
||||
return {"code": "auth-code", "state": "st"}
|
||||
|
||||
monkeypatch.setattr(auth_mod, "_xai_wait_for_callback", _wait_then_cancel)
|
||||
|
||||
def _must_not_persist(*args, **kwargs):
|
||||
raise AssertionError("tokens must not be persisted for a cancelled session")
|
||||
|
||||
monkeypatch.setattr(auth_mod, "_save_xai_oauth_tokens", _must_not_persist)
|
||||
monkeypatch.setattr(ws, "_add_xai_oauth_pool_entry", _must_not_persist)
|
||||
|
||||
# Should return cleanly without raising and without persisting.
|
||||
ws._xai_loopback_worker(session_id)
|
||||
assert session_id not in ws._oauth_sessions
|
||||
|
||||
|
||||
def test_unknown_pkce_provider_rejected_cleanly():
|
||||
"""A future PKCE provider without an explicit branch must NOT silently route to Anthropic.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user