mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-06 02:48:13 +00:00
fix(mcp): detect upstream invalid_grant by the RFC 6749 error field, not a body substring
The bridge refresh path decided whether an upstream token-endpoint rejection was invalid_grant by substring-matching the raw response body, so a rejection whose actual error is something else but whose error_description merely contains the string invalid_grant would false-match, map to invalid_grant, and trigger a needless authorization_code re-run Parse the RFC 6749 section 5.2 error object and compare the error field. A non-JSON body, or an error that is not invalid_grant, now propagates as the upstream error rather than being reinterpreted. The regression test drives an invalid_client rejection whose description contains the string invalid_grant and asserts it is not mapped, mutation-checked against the substring match
This commit is contained in:
parent
a9fac3c483
commit
9dbebf27a6
2 changed files with 64 additions and 1 deletions
|
|
@ -1338,6 +1338,21 @@ def _mint_refresh_envelope_value(
|
|||
return None
|
||||
|
||||
|
||||
def _upstream_oauth_error(response: httpx.Response) -> str | None:
|
||||
"""The RFC 6749 5.2 ``error`` code from an upstream token-endpoint error body, or ``None`` when the
|
||||
body is not a JSON object carrying a string ``error``. Reading the field beats substring-matching the
|
||||
raw text, which would false-match a code that only appears inside ``error_description`` (a false
|
||||
invalid_grant would trigger a needless authorization_code re-run)."""
|
||||
try:
|
||||
body = json.loads(response.text)
|
||||
except (ValueError, TypeError):
|
||||
return None
|
||||
if not isinstance(body, dict):
|
||||
return None
|
||||
error = body.get("error")
|
||||
return error if isinstance(error, str) else None
|
||||
|
||||
|
||||
async def exchange_token_with_server(
|
||||
request: Request,
|
||||
mcp_server: MCPServer,
|
||||
|
|
@ -1479,7 +1494,7 @@ async def exchange_token_with_server(
|
|||
is_bridge
|
||||
and grant_type == "refresh_token"
|
||||
and exc.response.status_code == 400
|
||||
and "invalid_grant" in exc.response.text
|
||||
and _upstream_oauth_error(exc.response) == "invalid_grant"
|
||||
)
|
||||
if upstream_rejected_bridge_refresh:
|
||||
verbose_logger.info(
|
||||
|
|
|
|||
|
|
@ -5176,6 +5176,54 @@ async def test_bridge_refresh_upstream_invalid_grant_maps_to_invalid_grant():
|
|||
assert json.loads(response.body)["error"] == "invalid_grant"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bridge_refresh_upstream_error_detection_parses_json_not_substring():
|
||||
"""The upstream invalid_grant detection parses the RFC 6749 5.2 error field, not a substring of the
|
||||
body. An upstream error whose code is not invalid_grant (here invalid_client, with the string
|
||||
invalid_grant only inside error_description) must NOT be mistaken for a dead refresh token, so it
|
||||
propagates as the upstream error rather than triggering a spurious authorization_code re-run."""
|
||||
import httpx
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.discoverable_endpoints import exchange_token_with_server
|
||||
from litellm.types.mcp import MCPAuth
|
||||
|
||||
server = _bridge_server(auth_type=MCPAuth.oauth_delegate)
|
||||
refresh_env = _mint_test_refresh_envelope(server_id=server.server_id, upstream_refresh="UP")
|
||||
|
||||
error_response = MagicMock()
|
||||
error_response.status_code = 400
|
||||
error_response.text = '{"error": "invalid_client", "error_description": "this is not an invalid_grant problem"}'
|
||||
error_response.raise_for_status = MagicMock(
|
||||
side_effect=httpx.HTTPStatusError("bad", request=MagicMock(), response=error_response)
|
||||
)
|
||||
fake_http_client = MagicMock()
|
||||
fake_http_client.post = AsyncMock(return_value=error_response)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client",
|
||||
return_value=fake_http_client,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints._revalidate_active_subject",
|
||||
new=AsyncMock(return_value=None),
|
||||
),
|
||||
patch("litellm.proxy.proxy_server.master_key", _BRIDGE_MASTER_KEY),
|
||||
):
|
||||
with pytest.raises(httpx.HTTPStatusError):
|
||||
await exchange_token_with_server(
|
||||
request=_bridge_mock_request(),
|
||||
mcp_server=server,
|
||||
grant_type="refresh_token",
|
||||
code=None,
|
||||
redirect_uri=None,
|
||||
client_id="dcr-client-123",
|
||||
client_secret=None,
|
||||
code_verifier=None,
|
||||
refresh_token=refresh_env,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_revalidate_key_subject_revoked_when_owner_scim_deactivated(proxy_globals):
|
||||
"""A key_hash refresh envelope whose key is still active but whose OWNING user was SCIM-deactivated must
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue