chore: LIT-3637 ad-hoc client release build off latest staging (do not merge) - #33609
chore: LIT-3637 ad-hoc client release build off latest staging (do not merge)#33609tin-berri wants to merge 16 commits into
Conversation
…e mcp_gateway_dcr flag The flag guarded no breaking change: the aggregate discovery lives at new /mcp-suffixed routes, the challenge only fires at aggregate scope, and the authorize/token/register/admission arms self-gate on the llm_dcrc_/llm_session_ prefixes. Bare-origin and per-server discovery are left exactly as they were, and a server literally named mcp keeps its own discovery via disambiguation, so turning it on for everyone changes nothing about existing flows.
…y challenges
Two RFC 9728 / 8414 discovery fixes on the aggregate front door, both raised by Bugbot on this PR
The aggregate authorization-server document at /.well-known/oauth-authorization-server/mcp used to
defer to a per-server row literally named "mcp", serving issuer {base} while the aggregate
protected-resource document advertises {base}/mcp as its authorization server. A spec client
following that chain fails the RFC 8414 issuer check and cannot sign in. The single segment /mcp is
now reserved for the aggregate so the issuer stays {base}/mcp and matches the protected-resource
document; a server named "mcp" keeps its standard two-segment discovery at
/.well-known/oauth-authorization-server/mcp/mcp
The 401 challenges built the resource_metadata URL as {base}/.well-known/oauth-protected-resource/mcp
with no SERVER_ROOT_PATH segment, but the routes are registered with the path-inserted root segment,
so a proxy mounted under a sub-path pointed DCR clients at a URL that 404s. Both the aggregate
challenge and the pre-existing per-server pass-through challenge now derive the path from one
well_known_root_suffix helper that the route registrations also use, so the advertised URL cannot
drift from the served route
tests/test_litellm/proxy/test_custom_proxy.py sets SERVER_ROOT_PATH at import time (its app mounts under a custom path) and never restores it, so in a shared shard the value leaks into the process. The discovery routes and the 401 challenges now read SERVER_ROOT_PATH to path-insert it where they previously ignored it, so a leaked value rewrites every resource_metadata URL and the exact-URL assertions in the delegate, pass-through, and aggregate challenge tests fail depending on shard order An autouse fixture clears SERVER_ROOT_PATH for the MCP discovery tests so they deterministically exercise the default root-mounted deployment; the tests that assert a sub-path deployment set the value explicitly within their own body. No assertion changed; the leak was invisible before only because the code ignored the variable
…w for the gateway front door
- atomic single-use guard (async_increment_cache) + reload-before-claim so a transient DB blip does not burn a valid code - PKCE verify over bytes so a non-ASCII code_challenge fails invalid_grant instead of raising a 500; validate code_verifier length (RFC 7636) - flag-off byte-identical for a server literally named mcp (AS well-known delegates to the named-server document) - connect flow is single-use (atomic jti claim) so a double-submit cannot mint two codes - extra=forbid on the sealed models; bound state length; drop unused request param and coarse dict on register - _reload_failure_response exhaustive match+assert_never; dedupe ReloadUserFailure with _KeyResolutionFailure - reject control/whitespace chars in the same-origin return_to
…le arm - _reload_admitted_user binds the user's org_id so the org-level MCP ceiling stays in force for a gateway session (a ceiling can only narrow; multi-org users are capped conservatively to their primary org) instead of being silently skipped - trim the NotSessionBearer arm comment to state it is simply unreachable
…ms for keyless admission
…ey absence A JWT-authenticated caller is also keyless with a user_id and, absent a team claim, no team_id, so gating the multi-team union on api_key-is-None silently broadened JWT MCP access to the union of every team the user belongs to. _reload_admitted_user now stamps MCP_ADMITTED_USER_SUBJECT_METADATA and the union fires only for that positive marker, so the gateway session and bridge user paths union while JWT and other keyless auth keep their prior behavior. Regression-tested.
…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
…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.
Greptile SummaryThis ad-hoc release build assembles the full LIT-3637 aggregate gateway DCR feature onto the current staging tip. It adds an OAuth 2.0 Dynamic Client Registration flow at the aggregate
Confidence Score: 4/5Marked 'do not merge' by the author; intended only as a build-from SHA. The new gateway DCR machinery itself is carefully built, but two pre-existing code paths are silently changed: bridge-flow admissions now get org_id bound (potentially narrowing MCP server access for bridge users in orgs), and all login paths now stamp exp on the UI session JWT (previously unbounded at the JWT layer). The new DCR flow, session tokens, and discovery routes are well-implemented with multiple security layers. The behavioral changes worth attention are the universal exp stamping on UI session cookies across all login flows and the org_id binding now reaching the existing bridge-flow admission path — both are changes to existing behavior without opt-in flags. Neither introduces incorrect data or broken security, but they could silently tighten or break deployments that relied on the old behavior. user_api_key_auth_mcp.py (the _reload_admitted_user org_id change affects the existing bridge path) and login_utils.py / proxy_server.py / ui_sso.py (the universal exp addition to UI session JWTs).
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/gateway_dcr_flow.py | New file: full OAuth 2.0 DCR flow for the aggregate /mcp endpoint. PKCE S256 enforced, sealed client records, single-use code guard via atomic cache increment, user re-validated before code claim. Well-designed with defense in depth. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/session_token.py | New file: HS256 session token minting and opening. Claim validation is fully Pydantic-gated with PyJWT validators disabled; all openers are total over hostile input. Size cap and prefix routing are correct. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/session_credentials.py | New file: producer/consumer helpers wrapping session_token.py. scrypt KDF domain label differs from bridge envelope labels, preventing cross-family key confusion. lru_cache on keys is correct since master_key is process-fixed. |
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Major changes: new session-bearer admission arm, admission fallback refactor, and team-union fan-out for admitted user subjects. The org_id binding in _reload_admitted_user now applies to the existing bridge path too, tightening access for bridge users in orgs. The N-team concurrent cache/DB fan-out in _get_allowed_mcp_servers_for_team is the most complex addition. |
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | New aggregate /mcp well-known routes registered before the parameterized ones (correct Starlette ordering), routing for aggregate authorize/token/register. well_known_root_suffix() replaces the inline ternary. sendBeacon endpoint /authorize/complete is properly POST-only. |
| litellm/proxy/auth/login_utils.py | encode_ui_session_jwt now stamps exp on all UI session JWTs across every login path; this is a universal behavior change — tokens that previously had no JWT-level expiry now expire at LITELLM_UI_SESSION_DURATION. |
| litellm/proxy/management_endpoints/ui_sso.py | Adds _is_same_origin_return_path (correct same-origin path validation) for the MCP DCR authorize round-trip, and honors it in _create_response. The google_login change is the only SSO-specific path updated; other providers go through _create_response which already handles it. |
| litellm/proxy/proxy_server.py | JWT minting in /login, /v2/login, /v3/login consolidated to encode_ui_session_jwt; no logic change beyond the exp addition delegated to that helper. |
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Adds well_known_root_suffix(), a single source of truth for the SERVER_ROOT_PATH insertion into .well-known paths, replacing duplicated inline ternaries across route registrations. |
| litellm/proxy/_experimental/mcp_server/server.py | Minor: 401 challenge URL now uses well_known_root_suffix() instead of a hardcoded segment, keeping the challenge URL consistent with the registered routes. |
| ui/litellm-dashboard/src/components/chat/ConnectFlowBanner.tsx | New component: finish step for gateway DCR connect flow. sendBeacon on pagehide is best-effort; the PERSERVER_CONNECTING_KEY guard correctly prevents it from firing during a per-server OAuth navigate-away, and the banner clears the key on mount for the return trip. |
| ui/litellm-dashboard/src/components/chat/MCPAppsPanel.tsx | Adds connectMode prop, oauthChecking state for skeleton loading, and renderConnectionIndicator helper. OAuth credential checks now run in parallel with tool-count fetches. Clean refactor. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_gateway_dcr_flow.py | Comprehensive new test file for all gateway DCR flow paths: registration, PKCE enforcement, single-use code guard, refresh token grants, and user revalidation. All mock-based, no real network calls. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Adds TestAggregateGatewayDcrChallenge, TestGatewaySessionAdmission, and TestUserSubjectTeamUnion test classes. Existing test mocks updated to add organization_id=None, correctly reflecting the new _reload_admitted_user return shape. |
Reviews (1): Last reviewed commit: "feat(ui): grey out client-forwarded and ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return UserAPIKeyAuth( | ||
| user_id=user_object.user_id, | ||
| user_role=user_object.user_role, | ||
| org_id=user_object.organization_id, | ||
| object_permission=object_permission, | ||
| object_permission_id=user_object.object_permission_id, | ||
| metadata={MCP_ADMITTED_USER_SUBJECT_METADATA: True}, |
There was a problem hiding this comment.
org_id binding now applies to the existing bridge-flow path
_reload_admitted_user is shared by both _admit_dcr_bridge_delegate (existing) and the new _admit_gateway_session. Adding org_id=user_object.organization_id to the returned UserAPIKeyAuth therefore silently applies the org-level MCP ceiling to every existing bridge-flow admission — not just new gateway sessions. Any bridge user who belongs to an org with MCP server restrictions will now have those restrictions enforced, whereas they were silently bypassed before (no org_id was set). This is a behavior change on an existing code path without a flag, and there is no mention of it in the PR description.
Rule Used: What: avoid backwards-incompatible changes without... (source)
|
|
||
|
|
||
| def encode_ui_session_jwt(returned_ui_token_object: ReturnedUITokenObject, master_key: str) -> str: | ||
| """Encode a UI session cookie JWT with a bounded ``exp``. | ||
|
|
||
| The single choke point every UI login path (SSO and username/password /login, /v2, | ||
| /v3) uses to mint the ``token`` cookie, so the cookie's lifetime is set in exactly one | ||
| place and cannot drift between paths. Without the ``exp`` the cookie is valid until the | ||
| master key rotates, and the session-cookie readers that require a bounded lifetime | ||
| (the MCP interactive sign-in) reject it. | ||
| """ | ||
| claims = {**cast(dict, returned_ui_token_object), "exp": _ui_session_exp_timestamp()} | ||
| return jwt.encode(claims, master_key, algorithm="HS256") | ||
|
|
||
|
|
||
| def create_ui_token_object( | ||
| login_result: LoginResult, | ||
| general_settings: dict, |
There was a problem hiding this comment.
exp globally, without a flag
encode_ui_session_jwt is now the single mint site for all login paths (/login, /v2/login, /v3/login, and SSO). Every new JWT gets exp = now + LITELLM_UI_SESSION_DURATION. Previously the JWT carried no exp claim, so its validity was bounded only by master-key rotation; the virtual key itself had the TTL but the JWT did not. Any integration or script that holds a UI session token and expects it to remain signature-valid beyond the configured LITELLM_UI_SESSION_DURATION will break after the next login. Because LITELLM_UI_SESSION_DURATION is configurable the impact is bounded, but the change affects all login flows at once without a user-controlled opt-in flag.
Rule Used: What: avoid backwards-incompatible changes without... (source)
| return [] | ||
| return await MCPRequestHandler._resolve_user_team_ids(user_api_key_auth.user_id, user_api_key_auth) | ||
|
|
||
| @staticmethod | ||
| async def _resolve_user_team_ids(user_id: str, user_api_key_auth: UserAPIKeyAuth) -> list[str]: | ||
| """The distinct team ids a user belongs to, from the live user record. Returns [] on | ||
| no DB, a missing user, or any resolution failure so a lookup blip narrows access | ||
| rather than raising; the caller's direct grants still apply.""" | ||
| from litellm.proxy.auth.auth_checks import get_user_object | ||
| from litellm.proxy.proxy_server import ( | ||
| prisma_client, | ||
| proxy_logging_obj, | ||
| user_api_key_cache, | ||
| ) | ||
|
|
||
| if prisma_client is None: | ||
| return [] | ||
| try: | ||
| user_object = await get_user_object( | ||
| user_id=user_id, | ||
| prisma_client=prisma_client, | ||
| user_api_key_cache=user_api_key_cache, |
There was a problem hiding this comment.
For an admitted user-subject (gateway session or bridge user), _get_allowed_mcp_servers_for_team first calls _resolve_user_team_ids (one get_user_object call) and then fans out to one get_team_object call per team via asyncio.gather. A user with many teams on first-request (cold cache) produces N+1 look-ups on every MCP tool call. The cache mitigates repeat hits, but teams with short TTLs or high user counts could see meaningful latency spikes in the hot path. Consider whether the user-team list can be sourced from the already-loaded user record returned by _reload_admitted_user (which also calls get_user_object) instead of a separate look-up in _resolve_user_team_ids.
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!
| if (finishedRef.current) return; | ||
| if (sessionStorage.getItem(PERSERVER_CONNECTING_KEY) === "1") return; | ||
| if (typeof navigator.sendBeacon === "function") { | ||
| navigator.sendBeacon(action, new URLSearchParams({ flow: flowHandle })); | ||
| } | ||
| }; | ||
| window.addEventListener("pagehide", autoFinishOnLeave); | ||
| return () => window.removeEventListener("pagehide", autoFinishOnLeave); |
There was a problem hiding this comment.
sendBeacon redirect behavior vs. UX claim
navigator.sendBeacon(action, new URLSearchParams({ flow: flowHandle })) sends the body as application/x-www-form-urlencoded, which FastAPI's Form(...) parses correctly. However, sendBeacon does NOT follow HTTP 303 redirects in a way that is meaningful — the browser fires the request and discards the response (including the redirect). This means after a successful sendBeacon, the DCR client's loopback never receives the code in the redirect response. The PR describes this as "best-effort … a convenience, not a consent gate", which is accurate, but it's worth confirming that the UX comment "Closing this tab finishes for you" matches what browsers actually do (the code reaches the client only if the browser coincidentally follows the redirect during teardown, which is not guaranteed).
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| 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: Authorization completes on page exit
An attacker can register a public DCR client, open its authorization URL for a signed-in victim, and close the popup to trigger this beacon. That mints and sends an authorization code to the attacker's registered redirect URI without the victim pressing "Finish connecting". Keep /authorize/complete behind an explicit user action rather than invoking it from pagehide.
| failure = await reload_user(opened.principal.user_id) | ||
| if failure is not None: | ||
| return _reload_failure_response(failure) | ||
| return _session_token_pair(opened.principal, keys, now) |
There was a problem hiding this comment.
Medium: Rotated refresh tokens remain reusable
An attacker who copies a refresh token can exchange the original bearer repeatedly for new access and refresh tokens, even after the legitimate client has refreshed it. The client_id check does not prevent replay because these are public clients. Atomically consume the refresh token's jti during exchange and reject subsequent uses, ideally revoking the token family when replay is detected.
PR overviewThis PR prepares an ad-hoc LiteLLM client release build from the latest staging branch, touching the dashboard connection-flow UI and the experimental MCP server dynamic client registration flow. It appears intended as a temporary release build rather than a merge-ready feature change. There are still open authorization and token-handling concerns in the current PR. The most serious issue allows an OAuth-style authorization flow to complete when a popup exits, potentially granting a code without the user explicitly finishing the connection. A second open issue leaves rotated refresh tokens reusable if copied, enabling continued token replay. No issues have been fixed or addressed yet, so the PR still carries meaningful authentication risk. Open issues (2)
Fixed/addressed: 0 · PR risk: 8/10 |
1 participant
Do not merge
This is an ad-hoc release build branch, not a change for review. It exists only to give a single commit SHA to build a client release from. The actual reviewable work is the LIT-3637 stack (#33174 -> #33182 -> #33188 -> #33189 -> #33190 -> #33191 -> #33192 -> #33197); those merge individually.
Build from
6c74e9b988a92da9af4de0404660929c65ff3e52(branchlitellm_lit3637_release)What it contains
Latest
litellm_internal_stagingplus the full LIT-3637 aggregate gateway DCR feature (all 16 commits through the connect grid, #33192) and the three follow-up fixes raised on #33174: reservemcpfor the aggregate authorization-server document, insertSERVER_ROOT_PATHinto the discovery challenge URL, and isolate the MCP tests from a leakedSERVER_ROOT_PATH. The docs/capstone PR (#33197) is intentionally excluded since a runtime build does not need it.Rebased onto the current staging tip; the one relocation conflict (
bridge_token_flowvs the DCR hardening commit) was resolved so no behavior was lost.Verification
Backend:
from litellm import *clean, and the MCP server suite passes (2282 passed; the only failures aretest_semantic_tool_filter.pyerroring on the optionalsemantic_routerdep that is not installed locally and passes on CI).Live end-to-end on a local proxy off this branch, 10/10: anonymous
POST /mcp/returns the RFC 9728 challenge; the protected-resource and authorization-server documents agree on{base}/mcp;/registermints a sealedllm_dcrc_client;/authorize303-redirects to SSO;/tokenrejects a bogus code with 400; and a validx-litellm-api-keystill returns 200.