feat(MCP/UI): add OAuth flow selector on the MCP edit page - #32298
Conversation
Greptile SummaryThis PR wires up the OAuth Flow Type selector on the MCP server edit page: the field was previously watched but never registered, leaving
Confidence Score: 5/5Safe to merge — the change is additive, all invariants from the previous stack PRs are preserved, and the new code paths are covered by tests. The backend additions are mechanical field propagations with a dedicated regression test. The frontend logic is straightforward: the No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds oauth2_flow to both the health-check table builder and _build_mcp_server_table so GET /v1/mcp/server responses carry the persisted flow; docstring on _resolve_oauth2_flow is tightened to emphasise security sensitivity |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx | Registers oauth_flow_type as a real Form.Item (was watched but never registered), fixes initialValues to use undefined for null-flow rows instead of defaulting to INTERACTIVE, and conditionally spreads oauth2_flow into the save payload only when auth_type is OAUTH2 and a flow is selected |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx | Two previously inverted tests are corrected to match the new registered-field behaviour (explicit rows re-persist their own value); two new tests cover admin selection of M2M and Interactive on legacy null-flow rows |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Adds test_build_mcp_server_table_carries_oauth2_flow to prevent regression on the field drop that previously blinded the dashboard to persisted flows |
Reviews (3): Last reviewed commit: "docs(mcp): flag _resolve_oauth2_flow as ..." | Re-trigger Greptile
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
9f341b1 to
f84a6f5
Compare
c63b7c1 to
622b57b
Compare
f84a6f5 to
993c161
Compare
622b57b to
dd86333
Compare
993c161 to
4ac467b
Compare
4eba310 to
f859105
Compare
The edit form had no flow selector: oauth_flow_type was watched but never registered, so isM2MFlow was always false in edit mode and the flow could only be changed over REST. That left the backfill's remediation for ambiguous legacy rows (client creds + token_url, no interactive signal, left unstamped) without a dashboard path The oauth2 section now opens with an OAuth Flow Type select. Explicit rows prefill their stored value and re-persist it on save; legacy null rows show a placeholder instead of a fake preselection, and an untouched save still writes nothing, so the form never guesses on the admin's behalf. Choosing Machine-to-Machine (M2M) persists oauth2_flow=client_credentials, choosing Interactive (PKCE) persists authorization_code, which is exactly the assertion the backfill warning asks for. Registering the field also brings the existing isM2MFlow gating in the edit form to life, so M2M rows stop showing the interactive-only token-validation fields Tests cover the prefill round-trip for both explicit values, the untouched null row writing nothing, and both selections persisting on a legacy null-flow row
_build_mcp_server_table and the health-check table builder dropped oauth2_flow when converting registry servers for GET /v1/mcp/server (list and by-id), so the dashboard never received the persisted flow: the edit page could not prefill the selector, M2M gating never activated, and the tools page classifier saw every oauth2 server as interactive regardless of the column. Found live while proving the edit-selector persistence path end to end; the write side was fine (PUT persists and the column reads back correctly), the read side was dropping the field at the conversion Both builders now carry oauth2_flow; regression test pins the conversion
…string
The prior wording ('not called directly by security sites') could read as if the
function has no security relevance, when it is the shape-inference engine both
request-time security helpers delegate to. Reword to state that plainly: it decides
M2M-vs-interactive for an unstamped row, must always be reached through
effective_oauth2_flow or resolve_oauth2_flow_for_request, and its M2M-shape branch
must not be weakened without accounting for those callers. Docstring-only; no logic
change
Raised by review on the stacked PR
4bb9036 to
a031073
Compare
|
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 a031073. Configure here.
|
@greptileai rereview |
…flow prefill The edit form derived the OAuth Flow Type select value from the stored oauth2_flow with a nested ternary duplicated at two call sites. Extract the mapping into a named helper in types.tsx (next to getMcpOAuthMode and the flow constants): client_credentials -> M2M, authorization_code -> Interactive, null/unset -> undefined so the select shows its placeholder instead of a guessed default. The tool-config call site keeps its null -> Interactive display fallback via a trailing ?? OAUTH_FLOW.INTERACTIVE, so behavior is unchanged. Adds unit tests for the helper; the existing prefill/save tests already cover the call sites
… edit page) An oauth2 MCP server whose oauth2_flow was never classified (legacy null row the backfill left ambiguous) now advertises that it needs attention instead of silently falling back. The server card shows an 'OAuth flow not set' warning tag for any auth_type=oauth2 server with no oauth2_flow, so admins can spot them in the list without opening each one. The edit page shows a warning alert directly under the new OAuth Flow Type selector while the flow is unset, and it clears the moment a flow is picked. Delegate (delegate_auth_to_upstream) servers are excluded from both: they authenticate via upstream PKCE passthrough and route to passthrough regardless of oauth2_flow, so the M2M-vs-interactive classification does not apply and prompting for it would be a false alarm. The edit page reads the delegate state from the watched switch when it is mounted and falls back to the stored value otherwise (useWatch returns undefined for an unmounted field). Also adds end-to-end coverage of the null-flow chain the selector depends on: build_mcp_server_from_table carries oauth2_flow=None verbatim into the GET response, so the dashboard maps it to undefined and shows the placeholder rather than a guessed default. Tests: backend null carry, the select prefill display for all three states, the edit-page warning show/hide/clear-on-select and delegate exclusion, and the card badge across oauth2/non-oauth2, stamped/unstamped, and delegate
a40fca4 to
e7a3d3b
Compare
Merging this PR will degrade performance by 10.79%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_completion_simple_message |
4.4 ms | 5 ms | -10.79% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing litellm_mcp_edit_flow_selector (e7a3d3b) with litellm_internal_staging (7d15f2f)
Relevant issues
Fifth of the oauth2_flow persistence sequence, stacked on #32292 (after #32283, #32288, #32290). The backfill in #32290 leaves ambiguous legacy rows (client creds + token_url, no interactive signal) unstamped and warns the admin to set oauth2_flow via the dashboard or PUT /v1/mcp/server; this PR makes the dashboard half of that remediation real, since the edit page previously had no flow control at all
Behavior
The dashboard edit page gains an OAuth Flow Type selector: explicit rows prefill and re-persist their stored value; legacy null rows show a placeholder with no fake preselection and an untouched save writes nothing; choosing Machine-to-Machine (M2M) persists client_credentials and Interactive (PKCE) persists authorization_code. Net effect: the remediation the backfill warning points at exists in the dashboard and explicit M2M rows stop rendering interactive-only fields
The final commit surfaces a warning when an oauth2 server has no persisted oauth2_flow (not delegate-auth). On the server card in the MCP servers list, a warning Tag "OAuth flow not set" appears with a tooltip explaining the remediation. On the edit page, a warning Alert appears below the OAuth Flow Type selector prompting the admin to choose M2M or Interactive. Delegate (PKCE passthrough) servers are not flagged since the classification does not apply to them
Linear ticket
N/A
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays 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
Dashboard (before / after)
Before, on the stack base, the OAuth section of the edit form jumped straight from Authentication to OAuth Client ID with no flow control; because oauth_flow_type was watched but never registered, isM2MFlow stayed false and the gating was dead code, so an M2M row still rendered the interactive-only fields (OAuth Scopes and below)
After, the section opens with an OAuth Flow Type select. A legacy null-flow row shows the Select OAuth flow placeholder with no fake preselection
An explicit client_credentials row prefills Machine-to-Machine (M2M), and registering the field activates the isM2MFlow gating so the interactive-only token-validation fields drop away
An explicit authorization_code row prefills Interactive (PKCE)
The four shots render the real MCPServerEdit form driven by the three oauth2_flow shapes this PR handles (null, client_credentials, authorization_code); the live-proxy steps below cover the persistence half over the real wire path
Unset flow warning (server card + edit page)
Before (commit
92b6eccd82, the helper refactor just before the warning commit), the card for a null-flow oauth2 server shows no warningAfter (commit
e7a3d3b3bb), the card shows a "OAuth flow not set" warning Tag with a tooltip explaining the remediationBefore, the edit form goes straight from the OAuth Flow Type selector to OAuth Client ID with no warning
After, a warning Alert appears between the selector and the client ID, prompting the admin to choose a flow
Live proxy walkthrough
On a live proxy backed by Postgres, continuing the #32290 runbook (an ambiguous legacy row named legacy_m2m exists with a null oauth2_flow and the startup warning names it)
Go to http://localhost:4000/ui/?page=mcp-servers, open legacy_m2m, Edit. The OAuth section now starts with an OAuth Flow Type select showing the Select OAuth flow placeholder (no fake preselection for a row that has no stored value)
Save without touching the select, then read the row back: oauth2_flow is still null (the form does not guess)
Edit again, pick Machine-to-Machine (M2M), Save. The read-back now shows client_credentials, and the next proxy restart no longer logs the ambiguity warning for this server
Open an interactive server that has a stored authorization_code stamp: the select prefills Interactive (PKCE), and saving re-persists the same value
Live run (stack tip, Postgres-backed proxy on localhost:4000)
The persistence chain behind the dropdown, exercised over the exact wire path the edit form uses (PUT /v1/mcp/server):
Proving this end to end surfaced a read-side gap fixed in c63b7c1: the registry-to-table conversions behind GET /v1/mcp/server (list, by-id, and health) dropped oauth2_flow, so the dashboard never received the persisted value and the selector could not prefill. After the fix the list returns it:
Type
🆕 New Feature
Changes
The edit form watched oauth_flow_type but never registered a field for it, so isM2MFlow was always false in edit mode, the M2M-vs-interactive gating in the form was dead code, and the only way to set or change a server's oauth2_flow after creation was the REST API. That gap mattered once #32290 started deliberately leaving ambiguous legacy rows unstamped with a warning telling admins to assert the flow
The oauth2 section of the edit page now opens with an OAuth Flow Type select mirroring the create form's options. Explicit rows prefill their stored value and re-persist it on save. Legacy null rows show a placeholder instead of a fake preselection, and an untouched save writes nothing, preserving the invariant from #32288 that the edit form never guesses a flow onto a row. Choosing Machine-to-Machine (M2M) persists oauth2_flow client_credentials; choosing Interactive (PKCE) persists authorization_code. Registering the field also activates the existing isM2MFlow conditional rendering, so explicit M2M rows stop showing the interactive-only token-validation fields
Tests cover the prefill round-trip for both explicit values, the untouched-null-row-writes-nothing invariant, and both selections persisting on a legacy null-flow row
The final commit (
e7a3d3b3bb) surfaces a warning for oauth2 servers with no persisted oauth2_flow. MCPServerCard checksauth_type === OAUTH2 && !oauth2_flow && !delegate_auth_to_upstreamand renders a warning Tag with tooltip. The edit form renders a warning Alert below the OAuth Flow Type selector when no flow is selected and delegate-auth is off; the warning clears once the admin picks a flow. Tests cover the warning rendering, clearing on selection, and exclusion for delegate-auth serversLink to Devin session: https://app.devin.ai/sessions/f03da2725ec94d28b3facf766871b102
Note
Medium Risk
Changes affect OAuth flow persistence and how M2M vs interactive auth is chosen in the admin UI and list API—security-relevant but scoped with tests and unchanged inference choke points.
Overview
Dashboard: The MCP server edit form now exposes an OAuth Flow Type select (M2M vs Interactive/PKCE). Stored
oauth2_flowvalues prefill and are re-sent on save; legacy rows withnullshow a placeholder, omitoauth2_flowwhen unchanged, and only persist after an explicit choice. Wiring the field activates existingisM2MFlowUI so M2M servers hide interactive-only controls.API:
_build_mcp_server_tableand health-check table construction now setoauth2_flowon list/by-id/health responses so the UI can read persisted flows after PUT.Docs/tests:
_resolve_oauth2_flowdocstring is tightened as security-sensitive; unit/UI tests cover API field propagation and save/persist behavior.Reviewed by Cursor Bugbot for commit a031073. Bugbot is set up for automated code reviews on this repo. Configure here.