ITADN

MCP: valid cached OAuth session is deleted when a remote context server start fails with a transport error (e.g. VPN not yet up)

#62326Openchrisgturk 创建于 14 天前
state:needs triage
C
chrisgturkcommented
## Summary When a remote (HTTP) MCP context server fails to **start** without a recorded 401 challenge, `resolve_start_failure` in `crates/project/src/context_server_store.rs` deletes the cached OAuth session from the system keychain — even when the failure is a pure transport error (connection refused, DNS failure, connect/request timeout) and the stored session is perfectly valid. For users whose MCP server is only reachable over a corporate VPN, this destroys a valid session on nearly every OS logon: Zed starts before the VPN tunnel is up, the `initialize` request fails at the transport layer, no `WWW-Authenticate` challenge is ever received, and the clearing heuristic fires. The user is then forced through the manual browser OAuth flow again — in our environment 2–3 times per day per machine. Log signature: ``` joby-gateway start failed with a cached OAuth session present; clearing it ``` ## Root cause `resolve_start_failure` gates the clearing on `server.auth_challenge()` being `None` plus a cached session existing: - On a genuine **401**, the transport records the challenge (`auth_challenge()` is `Some`), so the clearing branch is correctly skipped and the normal re-auth path runs. - On a **transport failure**, no HTTP response was ever received, so `auth_challenge()` is `None` — and the heuristic infers the cached session is "likely stale/expired and caused the failure" and deletes it. The code comment itself hedges with "likely". The predicate treats *absence of evidence that the token is good* as *evidence that the token is bad*. The only reliable positive signal of a bad token at this boundary is a 401 (`TransportError::AuthRequired` / recorded `WwwAuthenticate`); connection refused, DNS failure, and timeouts are indistinguishable from each other in the current code (all reach the function as untyped `anyhow` errors with no challenge recorded). The heuristic was introduced in #51768. There's an existing precedent for the correct pattern in the same file: `submit_client_secret` clears the client secret only on a typed, definite rejection (`OAuthTokenError.error == "unauthorized_client"`). ## Reproduction 1. Configure a remote HTTP MCP server that requires OAuth and complete the browser auth flow (session lands in the keychain). 2. Make the server unreachable at the transport level (disconnect VPN, firewall the host, or point DNS at a black hole). 3. Restart Zed (or otherwise trigger the context server start). 4. Observe the log line above; the keychain entry (`mcp-oauth:<canonical server uri>`) is deleted. 5. Restore network connectivity: the server now requires a full browser re-auth instead of connecting with the previously valid session. ## Expected behavior A transport-level start failure should leave the cached OAuth session untouched (land in the error state and retry later). The session should be cleared only on a definite auth rejection — a real 401/invalid_token — which the existing `resolve_auth_required` path already handles. If the heuristic is defending against a server that silently rejects a bad token without ever returning 401 (timeout instead), that case is indistinguishable from a slow/unreachable server, and the cost asymmetry favors keeping the session: a genuinely stale session self-corrects with one extra failed start once the server is reachable (real 401 → normal re-auth), while the current behavior destroys valid credentials on every network hiccup. Happy to submit a PR — the minimal fix is removing the clearing branch so non-401 start failures return `ContextServerState::Error` with the session intact. ## Environment - Zed on Windows (session store = Windows Credential Manager); the logic is platform-independent - Remote HTTP MCP server behind OAuth, reachable only via corporate VPN
0 条评论