feat(mcp): persist oauth2_flow explicitly on create instead of inferring it at read time - #32288
Conversation
…ing it at read time The UI create payload never carried oauth2_flow, so every UI-created oauth2 server persisted a null flow and relied on _resolve_oauth2_flow's field-shape inference at registry build. That inference cannot tell a DCR-registered interactive server (client creds + token_url, no persisted authorization_url) from an M2M server unless endpoint discovery succeeds first, and the dashboard cannot reproduce it at all because credentials are redacted in responses The create form now persists the selected flow for oauth2 servers: authorization_code for Interactive (PKCE), client_credentials for M2M. The REST create endpoints stamp an omitted oauth2_flow server-side with the same discriminator the legacy inference uses, run at write time where the payload carries plaintext credentials, so the decision is made once with full information and stored. Applied to the admin create, the BYOM submission, and the temporary session-server endpoints The edit form derives its flow display from oauth2_flow instead of token_url presence (token_url is present on authorization_code servers too, so it cannot distinguish M2M) and deliberately never writes oauth2_flow: it has no flow selector, so a write from edit could only erase an explicit value, including the authorization_code stamp the DCR flow persists. Regression tests pin all of this down Second step of persisting oauth2_flow at every write site so the legacy inference can eventually be deleted; the backfill for existing null rows lands next
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR ensures that every newly created OAuth2 MCP server has its
Confidence Score: 5/5Safe to merge — all three REST create paths correctly stamp a missing flow, the edit form no longer uses token_url as a flow discriminator, and edit saves are confirmed by test to never write oauth2_flow back. The change is narrowly scoped: a new stamp helper fills a missing field at write time, the dashboard create payload now includes the selected flow, and the edit form switches its display heuristic from token_url presence to the stored column. All three paths have regression tests that pin the exact before/after contracts. No auth logic, no critical request path, no schema migrations are touched in this PR. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Adds stamp_omitted_oauth2_flow helper (well-documented, correct M2M heuristic, explicit-value early return) and calls it at all three REST create sites in the right order (before required-field validation). |
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx | Adds MCP_OAUTH2_FLOW_INTERACTIVE import and spreads oauth2_flow into the create payload for oauth2 servers; correctly conditional on auth_type. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx | Switches oauth_flow_type initialisation and MCPToolConfiguration preview from token_url presence to oauth2_flow column; edit saves never include oauth2_flow (confirmed by new tests). |
| ui/litellm-dashboard/src/components/mcp_tools/types.tsx | Adds MCP_OAUTH2_FLOW_INTERACTIVE = "authorization_code" constant parallel to existing MCP_OAUTH2_FLOW_M2M. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | New standalone tests cover all stamp branches; no real network calls, no mock weakening detected. |
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx | New tests verify oauth2_flow inclusion in create payload for interactive, M2M, and non-oauth2 cases. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx | Three new tests assert oauth2_flow is absent from update payloads for null-flow, M2M, and authorization_code rows. |
Reviews (3): Last reviewed commit: "refactor(mcp): name the create-time flow..." | Re-trigger Greptile
This comment was marked as outdated.
This comment was marked as outdated.
…contract stamp_omitted_oauth2_flow with a dedicated explicit-value early return and the shape check renamed to has_m2m_shape, so the precedence (caller's oauth2_flow always wins, inference only fills an omitted field) reads directly off the code
Rows created before the write-side stamps carry a null oauth2_flow and rely on read-time field-shape inference, which cannot tell a DCR-registered interactive server (client creds + token_url, no persisted authorization_url) from an M2M server unless endpoint discovery succeeds first; on a transient discovery failure those servers flip to client_credentials for that registry load The backfill classifies each null oauth2 row once, at rest, ordered by signal strength: per-user token rows (only the interactive flow mints them, so this is definitive and catches the DCR-trap cohort), then a persisted authorization_url, then a persisted registration_url (DCR implies interactive; this covers registered-but-never-signed-in rows), then the M2M credential shape mirroring the legacy inference, else the interactive default that matches how needs_user_oauth_token treats a null flow. Every stamp is logged with the rule that fired and written with updated_by=oauth2_flow_backfill for auditability Runs in _init_mcp_servers_in_db before the registry load so the first build of the boot classifies from the column, is isolated so a failure cannot block server loading, and is idempotent: a healed fleet exits after one indexed query. This unblocks deleting the read-time inference for DB rows in the follow-up Third step of the oauth2_flow persistence sequence, after #32283 and #32288
…s config-only plus a logged backstop With every DB write site stamping oauth2_flow (#32283, #32288) and the startup backfill healing legacy null rows, the DB build no longer needs to re-derive the flow from field shape. build_mcp_server_from_table now reads the column verbatim via _explicit_oauth2_flow: unknown or null values resolve to None, which needs_user_oauth_token already treats as interactive, so an unstamped row degrades to the safe default instead of guessing M2M from a shape that a DCR-registered interactive server shares whenever discovery is down Field-shape inference survives in exactly two places. config.yaml-loaded servers keep it at load time: they are rebuilt from the config on every boot, so there is no row to backfill and load-time resolution is their write-time stamp. And the request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M row blocking caller Authorization forwarding (the P1 property); it now logs a warning whenever it actually fires, which is the fire-rate signal for deleting it once deployments have booted past the backfill Regression tests pin that the DB build does not infer M2M from the credential shape and reads an explicit column value verbatim Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
|
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 ceebdad. Configure here.
|
@greptileai rereview |
Rows created before the write-side stamps carry a null oauth2_flow and rely on read-time field-shape inference, which cannot tell a DCR-registered interactive server (client creds + token_url, no persisted authorization_url) from an M2M server unless endpoint discovery succeeds first; on a transient discovery failure those servers flip to client_credentials for that registry load The backfill classifies each null oauth2 row once, at rest, ordered by signal strength: per-user token rows (only the interactive flow mints them, so this is definitive and catches the DCR-trap cohort), then a persisted authorization_url, then a persisted registration_url (DCR implies interactive; this covers registered-but-never-signed-in rows), then the M2M credential shape mirroring the legacy inference, else the interactive default that matches how needs_user_oauth_token treats a null flow. Every stamp is logged with the rule that fired and written with updated_by=oauth2_flow_backfill for auditability Runs in _init_mcp_servers_in_db before the registry load so the first build of the boot classifies from the column, is isolated so a failure cannot block server loading, and is idempotent: a healed fleet exits after one indexed query. This unblocks deleting the read-time inference for DB rows in the follow-up Third step of the oauth2_flow persistence sequence, after #32283 and #32288
…s config-only plus a logged backstop With every DB write site stamping oauth2_flow (#32283, #32288) and the startup backfill healing legacy null rows, the DB build no longer needs to re-derive the flow from field shape. build_mcp_server_from_table now reads the column verbatim via _explicit_oauth2_flow: unknown or null values resolve to None, which needs_user_oauth_token already treats as interactive, so an unstamped row degrades to the safe default instead of guessing M2M from a shape that a DCR-registered interactive server shares whenever discovery is down Field-shape inference survives in exactly two places. config.yaml-loaded servers keep it at load time: they are rebuilt from the config on every boot, so there is no row to backfill and load-time resolution is their write-time stamp. And the request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M row blocking caller Authorization forwarding (the P1 property); it now logs a warning whenever it actually fires, which is the fire-rate signal for deleting it once deployments have booted past the backfill Regression tests pin that the DB build does not infer M2M from the credential shape and reads an explicit column value verbatim Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
…s config-only plus a logged backstop With every DB write site stamping oauth2_flow (#32283, #32288) and the startup backfill healing legacy null rows, the DB build no longer needs to re-derive the flow from field shape. build_mcp_server_from_table now reads the column verbatim via _explicit_oauth2_flow: unknown or null values resolve to None, which needs_user_oauth_token already treats as interactive, so an unstamped row degrades to the safe default instead of guessing M2M from a shape that a DCR-registered interactive server shares whenever discovery is down Field-shape inference survives in exactly two places. config.yaml-loaded servers keep it at load time: they are rebuilt from the config on every boot, so there is no row to backfill and load-time resolution is their write-time stamp. And the request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M row blocking caller Authorization forwarding (the P1 property); it now logs a warning whenever it actually fires, which is the fire-rate signal for deleting it once deployments have booted past the backfill Regression tests pin that the DB build does not infer M2M from the credential shape and reads an explicit column value verbatim Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
…32290) * feat(mcp): startup backfill stamping oauth2_flow on legacy null rows Rows created before the write-side stamps carry a null oauth2_flow and rely on read-time field-shape inference, which cannot tell a DCR-registered interactive server (client creds + token_url, no persisted authorization_url) from an M2M server unless endpoint discovery succeeds first; on a transient discovery failure those servers flip to client_credentials for that registry load The backfill classifies each null oauth2 row once, at rest, ordered by signal strength: per-user token rows (only the interactive flow mints them, so this is definitive and catches the DCR-trap cohort), then a persisted authorization_url, then a persisted registration_url (DCR implies interactive; this covers registered-but-never-signed-in rows), then the M2M credential shape mirroring the legacy inference, else the interactive default that matches how needs_user_oauth_token treats a null flow. Every stamp is logged with the rule that fired and written with updated_by=oauth2_flow_backfill for auditability Runs in _init_mcp_servers_in_db before the registry load so the first build of the boot classifies from the column, is isolated so a failure cannot block server loading, and is idempotent: a healed fleet exits after one indexed query. This unblocks deleting the read-time inference for DB rows in the follow-up Third step of the oauth2_flow persistence sequence, after #32283 and #32288 * fix(mcp): backfill leaves the ambiguous M2M shape unstamped instead of guessing client_credentials The credential shape (client_id + client_secret + token_url, no interactive signal) is shared by real M2M servers and DCR-registered interactive servers nobody has signed into: the DCR persist writes creds and token_url but not authorization_url or registration_url. Stamping client_credentials from that shape permanently mislabeled the interactive cohort, and once explicit the value is authoritative, so per-user traffic would run on the proxy's stored client credential with no discovery rescue and no backstop (it only guards null rows) The backfill now stamps only what it can prove. Interactive signals keep stamping authorization_code; the ambiguous shape is left null with an actionable warning naming the server and the fix (set oauth2_flow via the dashboard or PUT /v1/mcp/server). A true M2M row keeps working per-request through the security backstop while the warning nags; an interactive row keeps its Authorize button (null renders interactive), and one completed sign-in creates the per-user token that stamps it authorization_code at the next boot. Mirrors the config-level rule: M2M is asserted by a human, never guessed Raised by review on the PR * perf(mcp): batch the backfill stamps into one update_many per flow value The per-row update loop issued one DB round-trip per legacy row at startup; rows sharing a stamped value now go out as a single update_many, so the DB cost is constant in fleet size. Per-row logging keeps the rule that fired for each server Raised by review on the PR * fix(mcp): backfill stamps only rows still null at write time and counts only real OAuth token rows as sign-in proof Two review findings. The batched update_many matched on server_id alone, so an explicit oauth2_flow set between the backfill's read and its write (an admin PUT or a sign-in's DCR stamp landing in the boot window) would be overwritten with the inferred value; the where clause now also requires oauth2_flow to still be null, so an explicit value can never be clobbered under any interleaving And the per_user_tokens rule counted any LiteLLM_MCPUserCredentials row as proof of an interactive sign-in, but that table doubles as BYOK storage for user-supplied API keys; a BYOK-flavored row would have stamped an M2M-shaped server authorization_code. The rule now counts only rows whose payload decodes as a type oauth2 token via the existing _decode_oauth_payload discriminator, so bare keys, undecodable rows, and stale leftovers from a BYOK-to-oauth2 auth switch prove nothing Raised by review on the PR
…s config-only plus a logged backstop With every DB write site stamping oauth2_flow (#32283, #32288) and the startup backfill healing legacy null rows, the DB build no longer needs to re-derive the flow from field shape. build_mcp_server_from_table now reads the column verbatim via _explicit_oauth2_flow: unknown or null values resolve to None, which needs_user_oauth_token already treats as interactive, so an unstamped row degrades to the safe default instead of guessing M2M from a shape that a DCR-registered interactive server shares whenever discovery is down Field-shape inference survives in exactly two places. config.yaml-loaded servers keep it at load time: they are rebuilt from the config on every boot, so there is no row to backfill and load-time resolution is their write-time stamp. And the request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M row blocking caller Authorization forwarding (the P1 property); it now logs a warning whenever it actually fires, which is the fire-rate signal for deleting it once deployments have booted past the backfill Regression tests pin that the DB build does not infer M2M from the credential shape and reads an explicit column value verbatim Fourth step of the oauth2_flow persistence sequence, stacked on the backfill
…nfig; inference reduced to the request-time backstop (#32292) * refactor(mcp): read oauth2_flow verbatim from DB rows; inference stays config-only plus a logged backstop With every DB write site stamping oauth2_flow (#32283, #32288) and the startup backfill healing legacy null rows, the DB build no longer needs to re-derive the flow from field shape. build_mcp_server_from_table now reads the column verbatim via _explicit_oauth2_flow: unknown or null values resolve to None, which needs_user_oauth_token already treats as interactive, so an unstamped row degrades to the safe default instead of guessing M2M from a shape that a DCR-registered interactive server shares whenever discovery is down Field-shape inference survives in exactly two places. config.yaml-loaded servers keep it at load time: they are rebuilt from the config on every boot, so there is no row to backfill and load-time resolution is their write-time stamp. And the request-time backstop in _get_allowed_mcp_servers keeps a not-yet-backfilled M2M row blocking caller Authorization forwarding (the P1 property); it now logs a warning whenever it actually fires, which is the fire-rate signal for deleting it once deployments have booted past the backfill Regression tests pin that the DB build does not infer M2M from the credential shape and reads an explicit column value verbatim Fourth step of the oauth2_flow persistence sequence, stacked on the backfill * feat(mcp): deprecation warning when config-level M2M is inferred rather than declared A config.yaml oauth2 server whose credential shape decides client_credentials without an explicit oauth2_flow now logs a warning at load pointing the admin at the explicit declaration. First rung of the deprecation ladder: the docs make oauth2_flow the recommended path, the warning surfaces configs still relying on inference, and a future breaking release can turn it into a config validation error, at which point config-level shape inference dies entirely. Interactive omissions stay silent since the default matches inference there and nothing load-bearing is being guessed * feat(mcp)!: require explicit oauth2_flow for config-defined oauth2 servers A config.yaml server with auth_type oauth2 must now declare its flow; the load raises a config validation error naming both values and what each means: oauth2_flow: client_credentials for machine-to-machine (the proxy mints a shared token at token_url using client_id/client_secret) or oauth2_flow: authorization_code for interactive (per-user tokens via browser sign-in, including delegate_auth_to_upstream) This replaces the load-time shape inference for config servers entirely. The credential shape is genuinely ambiguous (a DCR-registered interactive server carries client creds + token_url with no authorization_url, identical to M2M), so the config asserts the answer instead of the proxy guessing it. With this, field-shape inference survives in exactly one place: the request-time security backstop, which is telemetry-gated for deletion BREAKING CHANGE: config-defined oauth2 MCP servers without oauth2_flow fail proxy startup with the error above. Add the one line to the server block; the error text says exactly which value to pick * test(mcp): pin the verbatim read for authorization_code alongside client_credentials Raised by review on the PR * fix(mcp): fail closed on the anonymous delegate gate for unstamped M2M-shaped servers Reading oauth2_flow verbatim (this PR) changed has_client_credentials from True to False for a legacy null-flow row that still carries the M2M credential shape. That value is what the anonymous upstream-delegate gate checks before skipping LiteLLM auth entirely, so an M2M-shaped delegate server that was never stamped would newly pass the gate: an unauthenticated caller could get it selected and then list/read upstream data using the client credentials the request-time backstop re-infers, running as LiteLLM's service account. This reopens the hole the gate's existing 'never delegate for M2M' guard was written to close The gate now resolves the flow (column first, shape fallback) instead of reading the bare column, mirroring the request-time backstop in _get_allowed_mcp_servers: both fail closed on the ambiguous M2M shape and are removed together once no null rows remain. A pure-PKCE delegate server (no stored credentials) resolves to a non-M2M flow and keeps its bypass, so the common delegate case is unaffected Tests: an unstamped M2M-shaped delegate server is denied the bypass (mutation-checked against the bare-column regression), and a pure-PKCE delegate server still bypasses Raised by review on the PR * fix(mcp): centralize the request-time oauth2_flow backstop across every security site Reading oauth2_flow verbatim made has_client_credentials unreliable for legacy null rows, and the backstop that compensates was applied at only one reader. Review found three more consequences of that per-site approach: - the anonymous-delegate allowlist in get_allowed_mcp_servers read the bare column, so an unstamped M2M-shape delegate server was surfaced to anonymous callers (High) - call_mcp_tool resolved allowed ids into MCPServer objects without the backstop, so a null-flow M2M-shape row kept has_client_credentials false on tool execution during a backfill gap, though the listing path was covered (High) - the request-time warning claimed the startup backfill would stamp the row next boot, but the backfill deliberately leaves the ambiguous M2M shape unstamped (Low) Rather than patch each site, introduce two helpers on MCPServerManager that are the single choke point for request-time resolution: effective_oauth2_flow(server) for the enum/boolean decisions (allowlist filter, anonymous-delegate gate) and resolve_oauth2_flow_for_request(server) for the egress object copy (listing and tool call). Both fail closed on the M2M shape and leave stamped rows and pure-PKCE rows untouched. The gate now shares effective_oauth2_flow instead of its inline resolution, and the corrected warning lives once inside resolve_oauth2_flow_for_request, so deleting the whole transitional layer later is a single-site change. Tests: helper unit coverage (stamped verbatim, null M2M-shape resolves, pure-PKCE stays None, stamped/pure-PKCE return the same object, corrected warning text), the anonymous allowlist excludes an unstamped M2M-shape delegate server, and the call path resolves the flow like the listing path. The two security-integration tests are mutation-checked against the bare-column regression. Raised by review on the PR
Relevant issues
Second of the sequence that persists oauth2_flow at every write site so the legacy field-shape inference in _resolve_oauth2_flow can eventually be deleted. Follows #32283 (the DCR-persist stamp); next is the backfill for existing null rows plus resolved-flow reads for the dashboard, then deletion of the inference
Heads up for #31772's rebase: that branch adds
oauth2_flow: isM2MFlow ? MCP_OAUTH2_FLOW_M2M : nullto both UI payloads and changes the same edit-form derivation lines this PR changes. Its create-payload line should yield to this PR's version (which persists authorization_code instead of null for interactive) and its edit-payload line should be dropped entirely: the edit form has no flow selector (oauth_flow_type is never a registered field there), so isM2MFlow is always false in edit and that line would rewrite every M2M row's flow to null on save and erase the stamp #32283 persistsBehavior
Every new oauth2 server records its flow at creation: the dashboard persists the dropdown choice (client_credentials or authorization_code) and REST creates that omit the field get it stamped server-side from the payload's plaintext credentials, with an explicit value always winning. The edit form's display derives from the column instead of token_url presence and edit saves never write the flow. Net effect: no new null-flow rows can be created and editing cannot corrupt a stored classification
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
Reproduction on a live proxy backed by Postgres (master key
sk-1234)Before this change the read-back shows
"oauth2_flow": null; after it shows"client_credentials"Shows
"oauth2_flow": "authorization_code"Live run (stack tip, Postgres-backed proxy on localhost:4000)
M2M shape without oauth2_flow, then interactive shape without oauth2_flow, both created over REST and read from the create response
Type
🆕 New Feature
Changes
The UI create payload never carried oauth2_flow, so every UI-created oauth2 server persisted a null flow and relied on _resolve_oauth2_flow's read-time field-shape inference. That inference cannot distinguish a DCR-registered interactive server (client creds + token_url, no persisted authorization_url) from an M2M server unless endpoint discovery succeeds first, and the dashboard cannot reproduce it at all because credentials are redacted in responses. This is the root of the misclassification family: the flow the admin explicitly chose at create time was being discarded and re-derived later with less information
The create form now persists the selected flow for oauth2 servers, authorization_code for Interactive (PKCE) and client_credentials for M2M. The REST create endpoints (admin create, BYOM submission, temporary session server) stamp an omitted oauth2_flow server-side using the same discriminator the legacy inference uses (token_url plus full client credentials and no authorization_url means M2M, anything else is authorization_code), run at write time where the payload carries plaintext credentials. An explicit oauth2_flow from the caller always wins
The edit form now derives its flow display from oauth2_flow instead of token_url presence; token_url is present on authorization_code servers too (it is what autonomous refresh authenticates against), so it cannot distinguish M2M, and the old heuristic misrendered interactive servers as M2M once token_url started being persisted for refresh. The edit form deliberately never writes oauth2_flow: it has no flow selector (oauth_flow_type is not a registered form field there), so a write from edit could only erase an explicit stored value, including the authorization_code stamp the DCR flow persists in #32283. Regression tests pin the create stamps, the server-side discriminator, and the edit-form preservation invariant
Note
Medium Risk
Touches OAuth classification on create paths and dashboard MCP auth UX; behavior change is intentional but mis-stamping could mis-route M2M vs interactive token handling until corrected.
Overview
OAuth2 MCP servers now persist
oauth2_flowat creation instead of leaving it null and inferring later from endpoint/credential shape.The dashboard create flow sends
authorization_codeorclient_credentialsfrom the flow selector. REST/admin create, user registration, and temporary OAuth session creates callstamp_omitted_oauth2_flow, which only fills a missing flow: M2M whentoken_url+ client id/secret exist and there is noauthorization_url, otherwiseauthorization_code; an explicitoauth2_flowis never overwritten.The edit form shows the flow from stored
oauth2_flow(nottoken_url) and does not includeoauth2_flowin update payloads, so saves cannot wipe a stored classification. Tests cover the stamp rules, create payloads, and edit preservation.Reviewed by Cursor Bugbot for commit ceebdad. Bugbot is set up for automated code reviews on this repo. Configure here.