fix(mcp): annotate connected-app reachability on the gateway connect page - #34867
Conversation
…page The MCP connect page resolved its server grid through the dashboard identity (admin shortcut or view_all returns the whole registry) while the gateway DCR session it sets up resolves servers as an admitted subject through grant sources only, so the page showed servers and tool counts the session is never served. GET /v1/mcp/server now accepts connected_app_view=true and stamps each returned server with connected_app_reachable, computed by the same _reload_admitted_user + get_allowed_mcp_servers pair the live session uses. The connect page requests the flag in connect mode and renders unreachable servers dimmed with a label, excluded from the Connected count and tool-count fetches. Failure to build the admitted set marks everything unreachable, which matches what such a session would actually be served. Default behavior without the param is unchanged for every existing consumer.
Greptile SummaryThis PR aligns the MCP gateway connect page with the servers available to the connected-app session.
Confidence Score: 5/5The PR appears safe to merge. The previously reported stale-detail and in-flight-selection failures are addressed by deriving details from the current server list, synchronously updating the current-list reference, rechecking reachability after asynchronous tool loading, and isolating cancellation state per load; no blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/models/mcp_server.py | Adds the nullable connected-app reachability response field while preserving the default API shape. |
| litellm/proxy/_experimental/mcp_server/ui_session_utils.py | Centralizes dashboard-session detection, admitted-user resolution, acting-user selection, and effective auth contexts. |
| litellm/proxy/_experimental/mcp_server/rest_endpoints.py | Applies the admitted-user principal to MCP REST list and call routes while preserving caller credentials for toolset-scoped requests. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Adds the opt-in reachability annotation using the same admitted-subject server resolver as gateway sessions. |
| ui/litellm-dashboard/src/components/networking.tsx | Extends the existing MCP server-list wrapper to request connected-app annotations. |
| ui/litellm-dashboard/src/components/chat/MCPAppsPanel.tsx | Filters unreachable connect-mode servers and prevents stale detail, selection, and list-load state after reachability changes. |
| ui/litellm-dashboard/src/components/chat/MCPAppsPanel.test.tsx | Covers connect-mode filtering, unsupported servers, detail invalidation, in-flight selection, and superseded list loads. |
Reviews (13): Last reviewed commit: "fix(mcp): write the connect-page server ..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…detail view A server the connect page marks unavailable could still be added through its detail view Connect action, so the selection could contain servers the connected-app session is never served. The unavailability decision now lives in one predicate, connectUnavailabilityLabel, consumed by the card indicator, the detail view action area, the toggle-on path, the oauth auto-select effect, and the Connected count, so no interaction path can disagree with the label. This also closes the same pre-existing hole for servers marked not supported on this connection, whose detail view likewise offered Connect, and removes a grandfathered nested ternary, ratcheting the eslint suppressions baseline down
|
Addressed the P1 in b644140 at the mechanism level. The unavailability decision now lives in one predicate, connectUnavailabilityLabel, and every surface that could add or count a server consumes it: the card indicator, the detail view action area (which renders the label instead of any Connect control), the toggle-on path in handleToggle, the oauth auto-select effect, and the Connected count, so no interaction path can disagree with the label. This also closed the same pre-existing hole for servers labeled not supported on this connection, whose detail view likewise offered Connect. Regression tests drive the detail view for both unavailable states in connect mode and assert the Connect action is absent, plus pin that it still renders outside connect mode; the detail-view gate is mutation-checked. The eslint suppressions baseline ratchets down by one grandfathered nested ternary removed in the restructure. |
|
The fix is solid. The centralization into A few observations on the implementation:
One minor note: in the "blocks connecting" test, The P1 is fully addressed. The fix is correct. |
|
bugbot run |
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 b644140. Configure here.
|
@greptileai rereview and score properly |
|
Update: re-QA'd at The interesting caller is a non-admin internal user whose management listing is a strict subset of what their gateway session can reach; The union branch fires, but the server it adds back cannot be connectedTheir plain integrations page lists two servers; In connect mode Clicking Connect on it dead-ends:
The three sets for that caller: Before the union commit the same user never saw Admin fixture is still exact at this HEAD: 3 servers, 12 toolsConnect page lists exactly acme/globex/initech with Earlier passes at ffc22f2 (hide instead of dim)
Revoking every grant reaches the new empty state, and it is honest; finishing that same flow the client is served zero tools: Regression: plain integrations page and the no-flag payload are unchangedTwo things noticed that predate this PR: on a cold proxy with no stored upstream credentials the connect-page tool counts render blank and authorizing a server over OAuth does not refetch its count, so "· N" only appears after a reload; and a user who claims an invitation link gets a UI session token without an Not exercised live: the sibling "Not supported on this connection" label (the fixture has only |
…ming them Product decision: the connect page should only show what a connected-app session will actually be served, so annotated-unreachable servers are now filtered out of the connect-mode list at fetch time rather than rendered dimmed. Unsupported auth types keep their existing dimmed label since they are a property of the server, not the caller. A user with zero reachable servers gets an explanatory empty state pointing at grants. The list filter is the single source: counts, tabs, auto-select, detail view, and tool-count fetches all derive from the already-filtered state
|
Product decision after review: in connect mode the page now hides annotated-unreachable servers entirely instead of dimming them (ffc22f2), so the connect surface lists exactly what the session will be served. The API contract is unchanged (opt-in connected_app_view param, connected_app_reachable field); the filter is applied once where the list is set, so counts, tabs, auto-select, the detail view, and tool-count fetches all derive from the filtered state. Unsupported auth types keep their dimmed label since that is a property of the server rather than of the caller, and a user with zero reachable servers gets an explanatory empty state. Tests updated: hiding is asserted in connect mode, full rendering asserted outside it, and the filter is mutation-checked |
…rver The connect view's membership came from the dashboard resolver with the admitted-subject answer only annotated on top, so a server reachable by the session but missing from the dashboard list would be invisible on the page; an under-report, the mirror of the bug this PR fixes. The connect view now unions in any session-reachable server the dashboard resolver did not list, built from the registry and redacted through the same ladder, so page membership equals the admitted set by construction in both directions
|
One more commit, 5ace001: the connect view now guarantees the invariant in both directions. Previously membership came from the dashboard resolver with reachability only annotated on top, so a server the session is served but the dashboard resolver missed would be invisible on the page (an under-report, the mirror image of the reported bug). The connect view now unions in session-reachable servers missing from the dashboard list, built from the registry and passed through the same redaction and sanitizer ladder, so page membership equals the admitted set by construction. Pinned by a test running as an internal user through the non-admin sanitizer, with the union arm mutation-checked |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
…credential The reachability view resolves through the owning user's admitted identity, so a caller-passed virtual key could use the param to enumerate servers beyond its own scope (ids, names, descriptions of the owner's wider grants). The view is now gated on is_ui_session_credential, a predicate factored out of resolve_ui_session_team_ids so the two user-identity widening sites share one trust boundary: the SSO-minted dashboard session token acting as its user. Any other credential gets the param as a no-op and the admitted resolver is never consulted for it
|
bugbot run |
…re, not per endpoint The list endpoint unioned in session-reachable servers itself while tool counts, Connect actions, and credential endpoints still authorized through build_effective_auth_contexts, whose contexts carry team grants but never the user row's own object permission; a user-granted server could render on the connect page while every interaction on it failed. The admitted-user context (the same auth a gateway session resolves with) is now appended inside build_effective_auth_contexts for UI session credentials, so the page list and every per-server action endpoint answer identically, and the list endpoint's one-off union is deleted. Caller-passed keys are still never widened (is_ui_session_credential gate inside the context builder) and a reload failure falls back to team contexts only
|
bugbot run |
|
@greptileai rereview |
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 dea4092. Configure here.
|
Re-ran the same QA at Same fixture and same caller as before: a non-admin internal user whose management listing used to be a strict subset of what their gateway session could reach ( The previously-403ing server is now genuinely connectable, and the page matches the client exactlyConnect on The credential actually lands, which matters because the panel marks a card connected optimistically: All three authorize, ending at "Connected (3)" with counts 5 / 4 / 3 and Then "Finish connecting", and the client's real Exact equality in both directions for the caller the earlier commits targeted; at the previous HEAD this same user saw 3 servers listed and was served 7 tools from 2. One behavior change worth confirming is deliberate: because the admitted context is no longer connect-view-only, that user's plain integrations page widened too, from two servers to three, and now reads "12 tools available". Gating holds, admin baseline unchanged, vault_srv still containedA virtual key for the same user still gets the unannotated payload with no widening, so the shared context did not leak to caller-passed keys: Admin still reaches "Connected (3)" with 5 / 4 / 3 and is served exactly those 12 tools, and the plain page still lists all four with
Still not exercised live: the sibling "Not supported on this connection" label (the fixture has only |
…t on tool routes Server reachability on the REST tool routes came from the widened context union while tool permission checks ran on the bare session key, which carries no object permission, so a dashboard user could invoke tools their user-level grant excludes. Rather than bookkeeping which context granted which server, the routes now choose one principal at the boundary: acting_user_auth swaps a non-admin UI session for the admitted-subject auth, the same identity a gateway session resolves with, so reachability, per-source fail-closed tool ceilings, rate limits, and billing attribution all bind through the admitted arms that already exist downstream. Admin sessions keep their operator view and caller-passed credentials are never widened. One swap point per route, no per-server principal picking, no parallel permission logic
|
Re-ran the QA at Same fixture and caller as before: a non-admin internal user with The tool list route and a real in-dashboard tool call both resolve as the admitted subjectConnect page counts for that user, and the same user's real And an actual invocation from the Tool Testing Playground of a tool only that user's own object permission grants; the bare session key carries no object permission, so this is the call that matters: The interesting check: a per-user tool ceiling now actually binds on both routesWorth noting that a plain happy-path run does not distinguish this fix from the bug, because the old behavior was over-permissive: a principal with no object permission gets The excluded sibling is refused on the call route: and the list route narrows from five tools to one: The allowed tool still executes and the card counts follow the ceiling (acme 1, "8 tools available"), so it restricts rather than breaks the route. Before this change both of these would have shown all five tools and let Admin operator view and virtual keys unchangedAdmin still sees all four servers with Admin connect mode is still exactly three servers with 5 / 4 / 3 and 12 served tools, and a caller-passed virtual key still gets the identical unannotated, unwidened payload with and without Two things I could not exercise. A Minor UI observation, not a blocker: connect-page counts now refetch on connect but render one step behind, so the server you just authorized shows a blank count until the next re-render; one reload settles all of them. |
|
@greptileai can you review this, give me a score and explain why |
…rver list The detail view held its own copy of the server object, so it outlived the list it came from. When a refetch dropped that server as unreachable, the open detail view kept rendering it and its Connect action still ran: the guard looked the server back up by id or name in the current list, found nothing, and fell through, because a missing target read as "nothing to block" rather than "no longer connectable" Store the selected server's id and derive the row from the list instead. A server the list no longer carries cannot be the detail view's subject, so the stale render, the stale tools query and the guard bypass stop being reachable states rather than being blocked one at a time. handleToggle now takes the server it is toggling, which deletes the lookup that could miss at all
Three call sites reloaded the admitted subject independently, and the management endpoint carried its own copy of the reload, the HTTPException swallow and the logging. admitted_user_context is now the only place that answers "what user identity does this dashboard session act as", and the connected-app reachability helper reads it, which also drops its dead empty-user_id branch That owner now carries the request's tracing span onto the admitted principal. _reload_admitted_user builds a fresh auth from the user row and has no span of its own, so swapping it in on the REST tool routes silently detached every downstream lookup and the tool-call logging from the request's trace Toolset scoping and the acting-as-user swap are mutually exclusive, so they now share one owner on the tools list route. The admitted subject resolves per grant source and a team source deliberately carries none of the caller's object_permission, so a toolset narrowing layered on top would evaporate on every team-granted server: the request would be admitted through the toolset grant and then served tools from servers the toolset never named. A request carrying a toolset name stays on the caller's own credential, exactly as it did before the swap
|
@greptileai can you review, give me a score and explain why |
|
bugbot run |
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 f5ecf60. Configure here.
…t stands Three continuations in the panel decided against state captured before their await and committed after it, so a reachability refetch landing in between could not be seen handleToggle validated the server at click time and then, once listMCPTools resolved, wrote its name into the selection whatever the list had since become; a server the refresh had dropped was selected anyway. It now re-asks connectableNow at the commit, and that predicate resolves the id against the current list, so absence fails closed instead of reading as nothing to block The load pipeline was worse, because its cancel flag was shared across runs: the successor's effect body reset it to false before the predecessor's fetch resolved, so a superseded load could still run setServers and put the dropped server back on the page outright. The flag is now a per-effect local that only that run's cleanup can clear, which is also what makes unmount stop the chunked tool-count loop again. The load passes its own liveness check down to the tool-count and oauth-status writes rather than having them consult a flag they share with every other run
|
Real, and fixed in c12085a, along with a second instance of it that was reachable by the same refetch You are right that Reading the rest of the panel's async surface for the same shape turned up the worse one. The load pipeline's cancel flag was shared across runs, and the successor's effect body reset it to Both are pinned by tests that hold the in-flight request open across the refetch and then release it: one asserts |
|
bugbot run |
|
Re-ran the QA at Why the two new guards are unproven (and what would make them testable)The stale-detail case needs a refetch that drops a server while the panel stays mounted. The list comes from one effect keyed on For the post-await re-check I armed a 120s delay at the HTTP edge on the tools list for an Both are reported as untested on purpose; neither is a defect I observed. Baseline at this HEAD: page and client agree exactly, and the ceiling still bindsNon-admin strict-subset user, connect page against that user's real gateway The rewired tool routes still enforce a per-user ceiling; with Toolset scope (API-level, since Admin baseline and virtual keys unchangedAdmin connect mode is exactly three servers with 5 / 4 / 3 and The admin plain page still lists everything with A caller-passed virtual key still gets the identical unannotated payload with and without Also unexercised: |
…itted connectableNow resolves a server id against serversRef, but that ref was a mirror kept in step by a passive effect, so it lagged the state it mirrored by however long React took to render and flush. A continuation resolving inside that window read the previous list: the commit-time reachability check would find a server the refetch had already dropped, call it connectable, and select it, which is the mismatch the check exists to prevent The lag was the whole defect, so the mirror is gone. commitServers writes the ref and the state together, at the one point the list is ever replaced, and the ref is now never older than the last committed list. Readers that want the newest answer (connectableNow, the oauth auto-select effect) get it; rendering still derives from state, so what is on screen is unchanged Pinned by a test that resolves the refetch and the in-flight Connect in the same tick, with no render flushed between them, which is the interleaving the earlier regression could not reach. The two prop mirrors are deliberately untouched: their staleness is inherent to appending to a parent-owned list from an async callback rather than caused by the mirror, and no reachability decision reads them
|
bugbot run |
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 a6d3b17. Configure here.
|
Re-ran the QA at The post-await guard, with the response actually succeeding this timeThe previous attempt was worthless because the revocation that creates the race also turned the delayed response into a 403, so Precondition, Connect clicked, response held, grant revoked, list re-committed while the request is still in flight. The server is gone from the grid and the open detail view has fallen back to the grid with no Connect control: Releasing the hold gives Negative control, so this is not vacuous: the identical sequence without the revocation does add it, badge 3 -> 4. Two honest caveats. The panel still has no user-facing in-place refetch (the sidebar Integrations push is deduplicated against the current route), so the re-commit was triggered by a native Baseline unchanged: the page and a real MCP client still agree exactlyCold load settles to 5 / 4 / 3, detail open/connect/disconnect/back behaves, the oauth auto-select still picks up connected servers, and switching in and out of connect mode never renders a count against a server the newer list does not contain. One cosmetic leftover, not new here: the header's aggregate "N tools available" is summed from a map that keeps entries for servers that have left the list, so it can briefly overcount while the grid and the per-card counts are correct. Admin view and virtual keysAdmin still sees every server with A caller-passed virtual key gets the identical unwidened payload with and without |
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Resolves LIT-4861
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Rig: proxy on localhost:4861 backed by real Postgres, four streamable HTTP MCP upstreams serving three tools each, and four servers registered through the API to exercise every grant channel: u1srv granted through the signed-in user's own object permission, t1srv granted through a roster team, a1srv flagged allow_all_keys, x1srv with no grant. The signed-in user is a proxy admin, matching the reported setup. The client leg is the full gateway DCR flow over the wire: /register, /authorize (303 to /ui/chat/integrations?connect_flow=...), POST /authorize/complete with the session cookie, /token with PKCE, then MCP initialize and tools/list with the minted session bearer
Before, at base bdf8f8c:
The page lists 4 connectable servers while the session it just created is served 3. This is the reported 253 vs 171 mismatch reduced to a rig
After, at e244403, same commands against the same database:
UI verification steps: run the dashboard dev server, start a gateway connect flow from any MCP client so the browser lands on /ui/chat/integrations?connect_flow=..., and observe that servers without a grant do not appear at all, while the plain Integrations page without connect_flow renders exactly as before
Type
🐛 Bug Fix
Changes
Two things a reviewer may ask. The API annotates rather than filters so the reachability field stays caller-relative and every other consumer of GET /v1/mcp/server is untouched; the connect page hides the annotated-unreachable rows client side, so the page never advertises a server the session cannot use, and the annotation still lands uniformly on the view_all and team_id branches. build_effective_auth_contexts now appends the admitted-user context for UI session credentials, so the page list, tool counts, Connect actions, and credential endpoints all resolve every server the user is granted through any channel identically; the page's membership equals the admitted set by construction and no row can render whose follow-up actions would be denied. On the REST tool routes a non-admin dashboard session is swapped for the admitted-subject principal at the route boundary (acting_user_auth), so tool permission ceilings, reachability, rate limits, and billing bind exactly as they do for a gateway session; admin sessions keep their operator view and caller-passed keys are never widened. The view is honored only for the dashboard's UI session credential, via is_ui_session_credential factored out of resolve_ui_session_team_ids so both user-identity widening sites share one trust boundary; a caller-passed virtual key gets the param as a no-op, so a scoped key can never enumerate its owning user's wider grants The keyless /ui/connect page (which renders the same panel without connect mode) and the enable_chat_ui gating of /ui/chat/integrations are pre-existing gaps tracked on the Linear ticket, kept out of this PR's scope
Three things changed after the last review round, all at the class rather than at the reported call
The connect page's detail view no longer keeps its own copy of the server object. It stores the selected server's id and derives the row from the list, so a server a refetch drops as unreachable cannot be the detail view's subject at all; the stale render, the stale tools query and the Connect action that fell through a missing-target lookup stop being reachable states instead of being blocked one at a time. handleToggle takes the server it is toggling, which deletes the lookup that could miss. Pinned by a test that opens the detail view, refetches a list without that server, and asserts the view falls back to the grid with no connect action anywhere; storing the object again fails it
admitted_user_context is now the single owner of "what user identity does this dashboard session act as". The management endpoint's private copy of the reload, the HTTPException swallow and the logging is gone, along with its dead empty-user_id branch, so the page annotation, build_effective_auth_contexts and the tool routes cannot answer that question differently. The owner also carries the request's tracing span onto the admitted principal, because _reload_admitted_user builds a fresh auth from the user row and has none of its own, so the previous round's swap had silently detached every downstream lookup and the tool-call logging from the request's trace
Toolset scoping and the acting-as-user swap are mutually exclusive, and they now share one owner on the tools list route. The admitted subject resolves per grant source and a team source deliberately carries none of the caller's object_permission, so a toolset narrowing layered on top would evaporate on every team-granted server: a non-admin dashboard user whose own object permission grants a toolset would be admitted through that grant and then served tools from servers the toolset never named. A request carrying a toolset name keeps the caller's own credential, exactly as it did before the swap. The equivalent composition on /toolset/{name}/mcp for a gateway session predates this PR, is untouched by it, and is tracked separately
A fourth followed from reading the rest of the panel's async surface. Three continuations decided against state captured before their await and committed after it, so a reachability refetch landing in between could not be seen. handleToggle validated the server at click time and then wrote its name into the selection whatever the list had since become, so a server the refresh had dropped was selected anyway; it now re-asks connectableNow at the commit, and that predicate resolves the id against the current list, so absence fails closed rather than reading as nothing to block. The load pipeline was the worse of the two because its cancel flag was shared across runs: the successor's effect body reset it to false before the predecessor's fetch resolved, so a superseded load could still run setServers and put the dropped server back on the page outright. That flag is now a per-effect local only its own cleanup can clear, which is also what makes unmount stop the chunked tool-count loop again, and the load passes its own liveness check down to the tool-count and oauth-status writes instead of having them consult a flag every other run shares. Both are pinned by tests that hold the in-flight request open across the refetch, and each mutation kills exactly its own test
QA runbook
Against the same rig, with the dashboard dev server on :3000 and the proxy on :4861
curl -s "http://localhost:4861/mcp-rest/tools/list?toolset_name=<a toolset the user's own object permission does not grant>" -H "Authorization: Bearer $UI_SESSION_KEY"should still be refused, the same as before this PR; the toolset request must not be admitted through the user's grant unioncurl -s "http://localhost:4861/v1/mcp/server?connected_app_view=true" -H "Authorization: Bearer $UI_SESSION_KEY"should return the same connected_app_reachable flags as before, and the served tool count over the real DCR flow should still equal the flagged-true set exactlyFinal Attestation
Note
Medium Risk
Touches MCP authorization identity resolution (dashboard vs admitted subject) and server listing semantics; behavior is gated to UI session credentials and connect mode, with broad test coverage, but mistakes could affect tool access or visibility for dashboard users.
Overview
Fixes the gateway connect flow showing MCP servers and tool counts that the minted OAuth session never receives.
Backend:
GET /v1/mcp/serveraccepts optionalconnected_app_viewand setsconnected_app_reachableon each row using the same admitted-subject resolver as live/mcpsessions (UI session credentials only; virtual keys are not widened).build_effective_auth_contextsnow also includes the admitted user context so listings match user-level grants. MCP REST tool list/call routes resolve non-admin dashboard sessions viaacting_user_auth(renamed from toolset-only scope), while toolset-scoped requests keep the caller credential so narrowing is not lost on team grants.Dashboard: In connect mode,
MCPAppsPanelrequests the new view, filters out unreachable servers, and hardens async behavior (detail view by server id, per-load cancellation, re-check after Connect) so refetches cannot leave stale UI or selections.Reviewed by Cursor Bugbot for commit a6d3b17. Bugbot is set up for automated code reviews on this repo. Configure here.