fix: resolve MCP server creation failures (schema drift, oauth2_flow mapping, session permissions) - #24941
fix: resolve MCP server creation failures (schema drift, oauth2_flow mapping, session permissions)#24941RoyVivat wants to merge 3 commits into
Conversation
…mapping, session permissions) - Restore approval_status and submission fields to root schema.prisma and litellm-proxy-extras schema (previously dropped by auto-generated sync migration) - Add migration to re-add the dropped columns with correct nullability and defaults - Map UI oauth_flow_type (m2m/interactive) → oauth2_flow (client_credentials/authorization_code) in create and edit MCP server forms before sending to backend - Allow non-admin users to create ephemeral session MCP servers (no DB write, no security risk) - Fix stale test expecting session endpoint to reject non-admins; add comprehensive new tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Greptile SummaryThis PR fixes three distinct MCP server creation failures: (1) restores Key changes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| litellm-proxy-extras/litellm_proxy_extras/migrations/20260401000000_restore_mcp_approval_columns/migration.sql | New migration that restores accidentally dropped columns using IF NOT EXISTS guards and a targeted backfill; well-documented with clear rationale. |
| litellm-proxy-extras/litellm_proxy_extras/schema.prisma | Restores source_url, approval_status (now nullable String? @default("active")), submission lifecycle fields, and adds an index; matches migration SQL. |
| schema.prisma | Root schema brought into sync with the proxy-extras schema by adding the same approval/submission lifecycle fields and index to LiteLLM_MCPServerTable. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Removes admin-only gate on session server creation (ephemeral, no DB write) and guards credential inheritance behind an admin-role check, directly addressing the security concern from prior review. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_creation_fixes.py | New comprehensive mock-based test file covering approval_status inclusion, oauth2_flow field acceptance, admin override, non-admin session creation, and non-admin credential-inheritance prevention. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Existing test renamed from rejects_non_admins → allows_non_admins to match the intentional behavior change; updated to verify the session server is created successfully rather than raising HTTPException. |
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx | Adds oauth_flow_type → oauth2_flow mapping block before payload construction; removes the UI-only field from the submitted payload. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx | Identical oauth_flow_type → oauth2_flow mapping block added to the edit form's submit handler; initializes oauth_flow_type from token_url presence (line 191) enabling correct round-trip mapping. |
| ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx | New test suite covers both M2M and interactive OAuth flow mapping for admin create and non-admin register paths; all mock-based, no real network calls. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx | Adds tests for oauth_flow_type mapping on the edit path, verifying client_credentials (token_url present) and authorization_code (no token_url) cases. |
Sequence Diagram
sequenceDiagram
participant UI as UI Form
participant BE as add_session_mcp_server
participant IC as _inherit_credentials
participant Cache as _temporary_mcp_servers
Note over UI: oauth_flow_type = "m2m" | "interactive"
UI->>UI: Map oauth_flow_type → oauth2_flow<br/>("client_credentials" | "authorization_code")<br/>delete oauth_flow_type
UI->>BE: POST /mcp/session {oauth2_flow, ...}
alt PROXY_ADMIN caller
BE->>IC: _inherit_credentials_from_existing_server(payload)
IC-->>BE: payload + stored secrets
else Non-admin caller
Note over BE: Skip credential inheritance
BE-->>BE: payload_with_credentials = payload (no secrets)
end
BE->>Cache: _cache_temporary_mcp_server(server, ttl=300s)
BE-->>UI: MCPServer (ephemeral, no DB write)
Reviews (3): Last reviewed commit: "fix: guard credential inheritance from n..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (restValues.oauth_flow_type) { | ||
| restValues.oauth2_flow = | ||
| restValues.oauth_flow_type === OAUTH_FLOW.M2M | ||
| ? "client_credentials" | ||
| : "authorization_code"; | ||
| delete restValues.oauth_flow_type; | ||
| } |
There was a problem hiding this comment.
The identical oauth_flow_type → oauth2_flow mapping block added to create_mcp_server.tsx received comprehensive tests in create_mcp_server.test.tsx, but mcp_server_edit.tsx has the same logic with no corresponding tests added to mcp_server_edit.test.tsx. If the mapping silently breaks during a refactor, there's nothing to catch it on the edit path.
Consider adding at least one test to mcp_server_edit.test.tsx verifying that when the edit form is submitted with oauth_flow_type: "m2m" the payload contains oauth2_flow: "client_credentials" (and vice-versa for "interactive").
| # Session servers are ephemeral (in-memory, ~5 min TTL, no DB write) so | ||
| # any authenticated user may create one. This lets non-admin users run | ||
| # the OAuth auth-test before submitting a server for review. | ||
| created_by = user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME | ||
| payload_with_credentials = _inherit_credentials_from_existing_server(payload) |
There was a problem hiding this comment.
Removing the PROXY_ADMIN check means any authenticated user can now call this endpoint repeatedly. Because the session cache likely has no per-user quota, a malicious (but valid) key holder could:
- Resource exhaustion — flood the in-memory session cache with thousands of short-lived entries until the proxy OOMs.
- SSRF probe — point the session server URL at an internal host (e.g.
http://169.254.169.254/) and inspect error messages that leak reachability, since session servers attempt to connect to the given URL when the OAuth flow is exercised.
Neither risk existed before, because only admins (who already have full access) could reach this path. Consider adding a lightweight per-user rate limit or at least an explicit URL allow-list / block-list before broadening this to all authenticated callers.
Rule Used: What: Fail any PR which may contains a security in... (source)
There was a problem hiding this comment.
This is a pre-existing concern at the API level, not specific to session servers. Any authenticated key can already flood numerous other endpoints. Neither issue is introduced by this diff in isolation, and reverting the restriction breaks the intended OAuth-test-before-submit workflow. The SSRF concern is real but belongs in validate_and_normalize_mcp_server_payload as a URL allowlist/blocklist, not as a role gate.
Addresses reviewer feedback: the create form tests covered the mapping but the edit form had no equivalent. Two tests added to mcp_server_edit.test.tsx verify that saving an OAuth2 server with token_url set produces oauth2_flow: "client_credentials", and saving one without token_url produces oauth2_flow: "authorization_code", in both cases without oauth_flow_type in the payload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| # any authenticated user may create one. This lets non-admin users run | ||
| # the OAuth auth-test before submitting a server for review. | ||
| created_by = user_api_key_dict.user_id or LITELLM_PROXY_ADMIN_NAME | ||
| payload_with_credentials = _inherit_credentials_from_existing_server(payload) |
There was a problem hiding this comment.
_inherit_credentials_from_existing_server runs unconditionally here and will copy sensitive credentials (OAuth client_secret, auth_value, AWS access/secret key, etc.) from any permanently-registered server whose server_id matches the caller-supplied value.
Before this PR only PROXY_ADMIN could reach this code path, so credential access was already gated. Now any authenticated user can pass an arbitrary server_id they know (or guess) and cause the endpoint to pick up that server's stored secrets. The credentials are redacted from the response (_redact_mcp_credentials(temp_record)) but the session server is cached in _temporary_mcp_servers with the full credentials intact, and subsequent tool-call / OAuth-flow requests that use the temp server_id will execute with those inherited secrets.
A minimal fix would be to skip inheritance if the caller is not an admin:
if LitellmUserRoles.PROXY_ADMIN == user_api_key_dict.user_role:
payload = _inherit_credentials_from_existing_server(payload)
# else: non-admin must supply explicit credentials; no inheritanceAlternatively, restrict _inherit_credentials_from_existing_server to only run when the caller supplies no server_id (forcing non-admins to always specify a fresh ID), which prevents piggybacking on an existing server's stored secrets.
Rule Used: What: Fail any PR which may contains a security in... (source)
…rs; fix migration backfill Security: _inherit_credentials_from_existing_server now only runs for PROXY_ADMIN callers. Non-admins opening a session server must supply their own credentials; they cannot inherit OAuth/AWS secrets from a permanently-registered server by supplying its server_id. Migration: backfill UPDATE also normalises rows with approval_status = 'approved' (written by the prior schema version that used @default("approved")) to 'active', so they are no longer excluded by the server manager's approval_status = 'active' filter. Tests: add test_add_session_mcp_server_non_admin_does_not_inherit_credentials to verify the credential isolation invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
2 participants
litellm-proxy-extras schema (previously dropped by auto-generated sync migration)
in create and edit MCP server forms before sending to backend
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@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).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes