Skip to content

fix: replace user api key auth with authorization or cookie for mcp server creation - #27190

Merged
yuneng-berri merged 2 commits into
BerriAI:litellm_internal_stagingfrom
dennishenry:mcp_oauth_key_fix
May 6, 2026
Merged

fix: replace user api key auth with authorization or cookie for mcp server creation#27190
yuneng-berri merged 2 commits into
BerriAI:litellm_internal_stagingfrom
dennishenry:mcp_oauth_key_fix

Conversation

@dennishenry

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/test_litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Delays 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)

CI status guideline:

  • 50-55 passing tests: main is stable with minor issues.
  • 45-49 passing tests: acceptable but needs attention
  • <= 40 passing tests: unstable; be careful with your merges and assess the risk.
  • Branch creation CI run
    Link:

  • CI run for the last commit
    Link:

  • Merge / cherry-pick CI run
    Links:

Screenshots / Proof of Fix

  • Before (401 on /authorize browser navigation):
{"error":{"message":"Authentication Error, No api key passed in.","type":"auth_error","param":"None","code":"401"}}
  • After:
works

Type

🐛 Bug Fix

Changes

Root cause: Commit 9deefc0 added dependencies=[Depends(user_api_key_auth)] to /v1/mcp/server/oauth/{server_id}/authorize and /v1/mcp/server/oauth/{server_id}/token. Both endpoints are reached via browser-based OAuth flows (/authorize via window.location.href redirect, /token via fetch()) neither of which sends an Authorization header. With master_key set, the standard auth raises 401.

Fix: Added a new _mcp_oauth_user_api_key_auth dependency in litellm/proxy/management_endpoints/mcp_management_endpoints.py that:

  1. Checks the Authorization header first (standard path — works for non-browser callers)
  2. Falls back to decoding the token cookie set by SSO login (a JWT signed with master_key containing the user's API key), enabling browser navigations to authenticate without an explicit Authorization header

This mirrors the existing cookie-decode pattern in byok_oauth_endpoints.py:104-124. The per-server access control added by 9deefc0 is fully preserved — non-admins are still blocked from temp-cache servers.

Files changed:

  • litellm/proxy/management_endpoints/mcp_management_endpoints.py - new _mcp_oauth_user_api_key_auth dependency; /authorize and /token switch to using it
  • tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py - two new tests: cookie-fallback path and Authorization-header-priority path

@CLAassistant

CLAassistant commented May 5, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

master_key,
algorithms=["HS256"],
# UI session cookies may omit exp; don't require it.
options={"verify_exp": False},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High: Expired session cookie accepted

verify_exp is disabled when turning the UI token cookie into an API key for the MCP OAuth endpoints. An attacker who obtained an old UI cookie can keep using it to authorize or exchange MCP OAuth tokens after the session expiration that protects normal cookie/JWT validation has passed; decode the cookie with expiration verification enabled and only exempt tokens that truly have no exp claim if that legacy path is required.

@veria-ai

veria-ai Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

MCP OAuth endpoint auth fallback reviewed

This PR changes the MCP OAuth authorize/token endpoints to authenticate with an Authorization header or, for browser navigation, a signed UI token cookie that is converted back into the contained API key before normal API-key validation and per-server access checks. I reviewed the changed helper, endpoint dependencies, surrounding server lookup authorization, and UI cookie issuance paths; no new security issue was found in the diff.


Status: 1 open
Risk: 2/10

@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...y/management_endpoints/mcp_management_endpoints.py 90.47% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@greptile-apps

greptile-apps Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a 401 error on MCP OAuth browser-navigation endpoints (/authorize and /token) by introducing a new auth dependency _mcp_oauth_user_api_key_auth that accepts either an Authorization header or a UI session cookie (set by SSO login). The pattern mirrors byok_oauth_endpoints.py:_user_id_from_session_cookie, and per-server access control from the original commit is preserved.

  • New _mcp_oauth_user_api_key_auth function checks the Authorization header first, then falls back to decoding the token cookie (JWT signed with master_key, login_method of \"sso\" or \"username_password\") to extract the embedded API key.
  • /authorize (GET) and /token (POST) endpoints swap user_api_key_auth for the new dependency; _user_api_key_auth_builder is then called with the resolved key.
  • Two focused unit tests cover the cookie-fallback path and the header-priority path, both fully mocked with no real network calls.

Confidence Score: 5/5

The core logic change is narrow and self-contained; per-server access control is fully preserved and the new dependency routes all auth through the existing _user_api_key_auth_builder.

The cookie-decode path mirrors an established pattern already present in the codebase. The fallback is guarded by both master_key presence and a login_method allowlist, and _user_api_key_auth_builder still does full token validation. The two previously flagged issues are tracked in open review threads and do not introduce new attack surface beyond what was already noted.

litellm/proxy/management_endpoints/mcp_management_endpoints.py — the unresolved items in prior review threads (exp verification, byok_session guard) are worth resolving before this merges.

Important Files Changed

Filename Overview
litellm/proxy/management_endpoints/mcp_management_endpoints.py Adds _mcp_oauth_user_api_key_auth dependency with cookie fallback for browser OAuth flows; two previously flagged security concerns (exp verification, byok_session guard) remain open in prior review threads.
tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py Adds two properly mocked async tests covering cookie-fallback and Authorization-header-priority paths; no real network calls.

Reviews (2): Last reviewed commit: "updated tests" | Re-trigger Greptile

Comment on lines +1524 to +1527
algorithms=["HS256"],
# UI session cookies may omit exp; don't require it.
options={"verify_exp": False},
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security verify_exp: False deviates from the reference security pattern

byok_oauth_endpoints.py:108-115 decodes the same cookie with options={"require": ["exp"]}, explicitly bounding a leaked session cookie's lifetime. This PR flips to verify_exp: False, accepting any master_key-signed JWT even if it carries an exp claim that has already passed. The comment acknowledges UI tokens currently omit exp (confirmed by ReturnedUITokenObject), but disabling expiry verification entirely means a cookie whose JWT was intentionally crafted with a past exp would still decode successfully — security then depends entirely on _user_api_key_auth_builder rejecting the embedded API key via DB lookup. Consider either adding exp to ReturnedUITokenObject and keeping exp-verification on, or at minimum using options={} (the default) so that when exp is present it is still checked.

# UI session cookies may omit exp; don't require it.
options={"verify_exp": False},
)
if decoded.get("login_method") in ("sso", "username_password"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Missing explicit byok_session token guard

byok_oauth_endpoints.py:119-120 explicitly checks if payload.get("type") == "byok_session": return None before inspecting login_method, with an in-code comment explaining that byok_session JWTs must never be replayable as UI cookies. The new function omits that check and relies implicitly on login_method being absent from byok_session tokens. While that currently blocks them, adding the explicit type guard would make the defence-in-depth explicit and resistant to future byok_session token schema changes.

@dennishenry

Copy link
Copy Markdown
Contributor Author

duplicate of #27106

@dennishenry dennishenry closed this May 5, 2026
@dennishenry dennishenry reopened this May 5, 2026
@yuneng-berri
yuneng-berri merged commit 73de892 into BerriAI:litellm_internal_staging May 6, 2026
83 checks passed
guangzhou pushed a commit to guangzhou/litellm that referenced this pull request May 7, 2026
…erver creation (BerriAI#27190)

* fix: replace user api key auth with authorization or cookie for mcp server creation

* updated tests
fzowl pushed a commit to fzowl/litellm that referenced this pull request Jun 24, 2026
…erver creation (BerriAI#27190)

* fix: replace user api key auth with authorization or cookie for mcp server creation

* updated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants