feat(ui): connect-flow interlude banner on the MCP apps grid for gateway DCR sign-in - #33192
Conversation
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge for the normal integrations path; the banner is only shown when connect_flow is present and the form POST is guarded by the backend cookie validation. The change is narrow and well-isolated. The banner renders without any backend round-trip, so a crafted URL with an arbitrary connect_client value will display misleading text until the user clicks Finish connecting and the backend rejects the mismatched cookie — that gap between display and enforcement is the main thing worth a second look before merge. ConnectFlowBanner.tsx — the connect_client display path; ConnectFlowBanner.test.tsx — the sensitive-field assertion.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/chat/ConnectFlowBanner.tsx | New component: renders the DCR sign-in interlude banner; uses a native form POST to /authorize/complete with only the flow handle as a field — design is intentional and well-commented. The connect_client URL param is passed straight to display without any origin-validation against the flow's registered client. |
| ui/litellm-dashboard/src/components/chat/ConnectFlowBanner.test.tsx | Three unit tests cover the form POST shape, the client origin display, and the null-origin fallback. Coverage is reasonable for the surface area. |
| ui/litellm-dashboard/src/app/chat/integrations/page.tsx | Reads connect_flow and connect_client search params and conditionally renders ConnectFlowBanner above the existing MCPAppsPanel; existing oauthReturn cleanup logic is untouched. |
| ui/litellm-dashboard/tsconfig.tsbuildinfo | TypeScript incremental build artifact; no code change to review. |
Reviews (1): Last reviewed commit: "feat(ui): connect-flow interlude banner ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| const ConnectFlowBanner: React.FC<Props> = ({ flowHandle, clientOrigin }) => { | ||
| const action = `${getProxyBaseUrl()}/authorize/complete`; | ||
| const clientLabel = clientOrigin ?? "the application"; |
There was a problem hiding this comment.
clientOrigin displayed without cross-checking the registered client
The connect_client URL parameter is shown verbatim as the identity of the connecting service ("Connect your MCP servers to {clientLabel}"). Because connect_flow is intentionally left in the URL, anyone who knows or guesses an opaque flow handle can craft a link with an arbitrary connect_client value and present a misleading "Connect to YourBank" banner to a signed-in user. The POST itself will fail at /authorize/complete once the backend validates the cookie, but the banner still renders before that check fires.
If the /authorize step already stores the client origin in the sealed cookie, the backend could echo it back (e.g., as a short-lived signed display hint) so the UI can show a value that matches the actual DCR registration rather than trusting the URL param.
| expect(form.innerHTML).not.toContain("token"); | ||
| }); |
There was a problem hiding this comment.
expect(form.innerHTML).not.toContain("token") passes as long as the literal string "token" never appears anywhere in the rendered HTML — including class names, aria labels, or comments. A hidden field named "auth_token" would be caught, but "access_code" or "secret" would not. Enumerating all <input> elements by querySelectorAll('input') and asserting exactly one name="flow" field would give the same intent with a tighter, name-based assertion.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5daf091 to
8e5d0a6
Compare
1707d3e to
08a9cb0
Compare
Claude Code gateway DCR runtimeRan PR head
|
8e5d0a6 to
1010c32
Compare
08a9cb0 to
4a04e6c
Compare
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| */ | ||
| const ConnectFlowBanner: React.FC<Props> = ({ flowHandle, clientOrigin }) => { | ||
| const action = `${getProxyBaseUrl()}/authorize/complete`; | ||
| const clientLabel = clientOrigin ?? "the application"; |
There was a problem hiding this comment.
Medium: Misleading OAuth client identity
A malicious DCR client can register a redirect such as https://claude.ai@evil.example/callback. The banner displays that raw authority as the client identity, while the browser sends the authorization code to evil.example, allowing the attacker to trick a user into issuing gateway access and refresh tokens to the malicious client. Parse the value with the browser URL implementation and display its normalized origin instead.
| const clientLabel = clientOrigin ?? "the application"; | |
| const clientLabel = (() => { | |
| if (!clientOrigin) return "the application"; | |
| try { | |
| const parsed = new URL(clientOrigin); | |
| return parsed.protocol === "https:" || parsed.protocol === "http:" ? parsed.origin : "the application"; | |
| } catch { | |
| return "the application"; | |
| } | |
| })(); |
PR overviewThis PR adds a connect-flow interlude banner to the LiteLLM dashboard for gateway DCR sign-in from the MCP apps grid. The banner presents the requesting application context and participates in completing or cancelling the authorization flow. There are two open security issues in the OAuth/DCR interlude flow, with none addressed yet. The most significant issue is that the authorization flow can complete without an explicit user confirmation, allowing a malicious registered client to obtain user tokens if it drives an already-authenticated user through the flow. A second issue can make the requesting client identity misleading by displaying an unnormalized redirect authority. The PR should be considered high risk until completion is gated on an explicit user action and the client identity display is normalized. Open issues (2)
Fixed/addressed: 0 · PR risk: 8/10 |
…nnect-status flash In the gateway DCR connect flow the apps grid now reads as "authorize your servers" rather than a chat feature; the connectMode prop drops the Beta badge, the "use in chat" subtitle, and the tool-count chrome Closing the connect tab now best-effort finishes the flow via navigator.sendBeacon to /authorize/complete, so the gateway authorization code still reaches the client's loopback without an explicit click; the explicit "Finish connecting" button stays as the reliable path. The beacon is skipped while a per-server authorize is navigating away and after the button was pressed, so it never double-delivers or fires mid-authorize Authorized servers previously flashed "Connect" for a second before flipping to "Connected" because the per-user credential checks ran only after the whole tool-count fetch finished. They now fire in parallel with the tool-count load, and each card shows a skeleton in the button slot until its status resolves, so the state never flips under the user
| if (finishedRef.current) return; | ||
| if (sessionStorage.getItem(PERSERVER_CONNECTING_KEY) === "1") return; | ||
| if (typeof navigator.sendBeacon === "function") { | ||
| navigator.sendBeacon(action, new URLSearchParams({ flow: flowHandle })); |
There was a problem hiding this comment.
High: OAuth flow completes without user confirmation
A malicious DCR client can send an already-authenticated user through its authorization URL and then receive a redeemable code when the user merely closes or navigates away from this page. Because the client controls the PKCE verifier, it can exchange that code for access and refresh tokens representing the user; only an explicit finish action should call /authorize/complete.
…y connect flow On the aggregate gateway connect flow the client holds only an identity-only session bearer, and upstream credentials are resolved server-side from the per-user vault, which is only populated by interactive authorization_code (oauth2). The client-forwarded modes (true_passthrough, oauth_delegate) need the caller to present the upstream Authorization per call, and oauth2_token_exchange (OBO) needs the caller's own IdP token as the exchange subject; the session bearer is neither, so a tool call to those servers can never complete on this connection. Rather than let them look connectable and then 401, the grid greys those servers and labels them "Not supported on this connection" when rendered in connect mode. Outside the connect flow the normal integrations page is unchanged, since the client forwards its own token there and those modes work. The classification lives in a shared isUnsupportedOnGatewayConnect helper next to isClientForwardedTokenMode so the UI gate and the auth-mode taxonomy cannot drift.
1dedeaa
into
litellm_lit3637_session_admission
1 participant
Relevant issues
Stacked on #33191 (multi-team union) -> #33190 -> #33189 -> #33188 -> #33182 -> #33174. The base of this PR is
litellm_lit3637_team_union; review and merge after thoseLinear ticket
Part of LIT-3637 (PR 5 of the stack: the magic-URL interlude that turns the apps grid into the finish step of the gateway DCR sign-in)
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This is a UI-only change. To see it end to end against a live proxy (aggregate gateway DCR is on by default, no flag):
npm run devinui/litellm-dashboard, served on port 3000)http://localhost:4000/ui/chat/integrations?connect_flow=<handle>&connect_client=https://claude.ai/authorize/complete(a form POST for the button, a sendBeacon for the close) and the client receives the gateway authorization code at its redirect URI, completing sign-inThe unit tests assert the finish action is a full-page form POST carrying only the flow handle (no token, code, or secret), that closing the tab fires a sendBeacon to the same endpoint, that the beacon is suppressed while a per-server authorize is in flight and after the button was pressed, that the client origin is shown, and that it falls back to a generic label when the origin is absent
Type
🆕 New Feature
Changes
The gateway DCR authorize (PR #33189) sends a signed-in user to
/ui/chat/integrations?connect_flow=<handle>to authorize servers before finishing sign-in. This adds the interlude that page needs: whenconnect_flowis present, aConnectFlowBannerrenders above the apps grid, naming the client the user is connecting to and offering a single finish actionIn this connect context the grid drops its chat-oriented chrome so it reads as "authorize your servers" rather than a chat feature: the
connectModeprop hides the Beta badge, the "browse tools, authenticate once, use in chat" subtitle, and the tool-count summary, leaving just the server cards you click into to see tools and connect. With noconnect_flowparam the page is byte-identical to today, so the normal browse-and-connect experience is untouchedFinish happens two ways. The explicit "Finish connecting" button is a native HTML form POST to the proxy's
/authorize/complete, not a fetch: that endpoint 303-redirects the browser back to the DCR client's own redirect URI with the gateway authorization code, and only a full-page navigation carries the HttpOnly per-flow cookie and follows that redirect. Closing or navigating away from the tab now also best-effort finishes, vianavigator.sendBeaconto the same endpoint, so the code still reaches the client's loopback without an explicit click and the browser follows the 303 the same way. This is a convenience, not a consent gate, since consent already happened at sign-in. It is skipped while a per-server authorize is navigating the page away (tracked by a short-lived sessionStorage flag) and after the button was pressed, so it never fires mid-authorize or double-delivers the code. The form and the beacon both carry only the opaque flow handle; everything sensitive lives in the sealed cookie the authorize step setAuthorized servers used to flash their Connect button for about a second before flipping to Connected, because the per-user credential checks only ran after the whole tool-count fetch had finished. The credential checks now fire in parallel with the tool-count load, and each OAuth card shows a skeleton in its button slot until its own status resolves, so the connect/connected state never flips under the user. Batching the status into the
/v1/mcp/serverresponse is a separate backend/schema optimization still left out to keep this change UI-onlyQA runbook
Follow the numbered steps above. Also confirm that visiting
/ui/chat/integrationswith noconnect_flowparam shows no banner, keeps the Beta badge and tool-count chrome, and behaves exactly as beforeFinal Attestation