fix(mcp): auth-type handling, OAuth-pending UX, safe diagnostics + HTTP/2#5757
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Client & OAuth flow: UX: Disconnected OAuth servers show “OAuth authorization required” in the list, detail status, and refresh action unless the refresh returned a concrete error (still Failed). Logging & transport: Connect/transport/ Reviewed by Cursor Bugbot for commit b856239. Configure here. |
Greptile SummaryThis PR improves MCP authentication, OAuth handling, diagnostics, and transport behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(mcp): make pinned Agent teardown ide..." | Re-trigger Greptile |
d498679 to
0424d18
Compare
|
@cursor review |
0424d18 to
ae7f8b6
Compare
|
@cursor review |
|
@cursor review |
MCP servers are commonly behind HTTP/2 fronts (CDNs, cloud LBs), but the SSRF-pinned undici Agent is HTTP/1.1-only unless it opts into h2 via ALPN. Add an allowH2 option to createPinnedFetch (default false, so LLM-provider callers are unchanged) and centralize the MCP h2 decision in one createPinnedMcpFetch routed through the transport, OAuth probe, and SSRF-guarded fetch. Pinning is unaffected: the pinned lookup forces every connection to the resolved IP.
…r logging - Classify a non-OAuth UnauthorizedError as an auth failure instead of an OAuth redirect, so static-bearer servers that merely advertise OAuth stop being diverted into the OAuth flow (fixes the class of server that couldn't connect). - Reset a server to disconnected when it is switched to OAuth, so it can't falsely read as connected before completing its auth flow. - Surface OAuth-pending state as 'OAuth authorization required' in the server list and refresh action instead of a generic 'Not Connected'. - Return an actionable 422 for OAuth servers that don't support dynamic client registration. - Redact error messages/cause/session-ids from MCP transport/connect logs via a shared getMcpSafeErrorDiagnostics helper.
… on auth-type change
…at client-id/secret setup
…oy on cancel/failed connect
c1c328e to
b856239
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b856239. Configure here.
Summary
Minimal, focused set of MCP client fixes — the genuinely-good changes carved out of #5754 (whose large ordered-publication consistency layer was reverted as disproportionate; see below). Two clean commits, no fencing/CAS machinery.
UnauthorizedErroris now classified as an auth failure instead of an OAuth redirect, so static-bearer servers that merely advertise OAuth stop being diverted into the OAuth flow (fixes the class of server that couldn't connect, [BUG] MCP server "discover" step ignores configured static Authorization header, always requires OAuth #5665).disconnected(it hasn't completed its auth flow), matching the create-path invariant, so it can't falsely read asconnected.getMcpSafeErrorDiagnostics.Not in scope
This does not fix the dominant
-32001 tools/listtimeouts (connect-per-op with no connection reuse) — that's the next PR (turn-scoped connection reuse).Type of Change
Testing
416 MCP/security tests pass; tsc clean; biome clean; adversarial review passed.
Checklist