fix(mcp): preserve per-user OAuth Authorization over signer JWT on tools/call (#31977) - #32129
Conversation
Greptile SummaryThis PR fixes a bug on the
Confidence Score: 4/5The core fix is correct and well-tested; the two findings are observability and directory-convention issues that do not affect runtime behaviour. The precedence logic in the new helper correctly mirrors the tools/list path and is backed by targeted unit tests. The main risk is that the old warning log for JWT-overrides-static-Authorization was removed without replacement, reducing admin visibility. The test file is also placed in a directory that does not match the existing _experimental/ convention, which could cause it to be missed by targeted test runs. tests/test_litellm/proxy/experimental/mcp_server/test_hook_headers_precedence.py should be verified to be discoverable by the CI test suite, as it is in a different directory from the rest of the MCP server tests.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds _merge_hook_extra_headers_with_precedence helper and replaces the old warn-and-clobber block in _call_regular_mcp_tool; logic is correct for the common cases, but the old warning that fired when the JWT overwrote a static Authorization is now silently dropped. |
| tests/test_litellm/proxy/experimental/mcp_server/test_hook_headers_precedence.py | New pure-unit test file covering the 7 main precedence scenarios; no real network calls; placed in experimental/ (no underscore) while existing MCP tests live in _experimental/ (with underscore). |
Reviews (1): Last reviewed commit: "fix(mcp): preserve per-user OAuth Author..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| headers_to_apply = { | ||
| k: v for k, v in headers_to_apply.items() if not (isinstance(k, str) and k.lower() == "authorization") | ||
| } | ||
|
|
||
| merged.update(headers_to_apply) |
There was a problem hiding this comment.
The old code emitted a warning-level log entry whenever the hook JWT was about to overwrite an existing (static) Authorization in extra_headers. That entry was the only signal an admin had that two credentials were competing. The new helper only emits a debug message on the preserve-OAuth branch; when the JWT does win (i.e. preserve_existing_authorization is False and a static Authorization gets overwritten), no message is logged at all. An admin who configured static_headers.Authorization will have no visibility into why their credential is being silently replaced by the JWT signer.
| @@ -0,0 +1,84 @@ | |||
| """ | |||
There was a problem hiding this comment.
The new file lives under tests/test_litellm/proxy/experimental/mcp_server/ (no leading underscore), while the rest of the MCP server test suite—including test_mcp_hook_extra_headers.py cited in the PR description—is under tests/test_litellm/proxy/_experimental/mcp_server/ (with underscore, matching the source path). This may cause the file to be missed by test-discovery configurations that glob for _experimental/ or to diverge from future test reorganisations.
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!
63e9db0 to
ad48a54
Compare
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…ols/call When mcp_jwt_signer runs as a pre_mcp_call guardrail, tools/call to an OAuth-backed MCP server overwrote the user's OAuth Authorization header with the signed JWT, so the upstream server rejected the call. tools/list already skips JWT injection when an Authorization credential exists (per BerriAI#28227); the tools/call path only warned, then clobbered it. Merge hook headers via a new _merge_hook_extra_headers_with_precedence helper that preserves a per-user OAuth Authorization (server auth_value, per-user mcp_auth_header, or a caller-supplied non-static Authorization in extra_headers), while still letting the signer JWT override an admin static Authorization. Mirrors the tools/list precedence. Closes BerriAI#31977
ad48a54 to
64efa88
Compare
1 participant
What / Why
Closes #31977.
When
mcp_jwt_signerruns as apre_mcp_callguardrail,tools/callto an OAuth-backed MCP server failed: the signer's JWT overwrote the user's OAuthAuthorizationheader, so the upstream server rejected the request.tools/listalready handles this correctly (per #28227) — it skips JWT injection when an Authorization credential is already present, because per-user OAuth must take precedence. Thetools/callpath (_call_regular_mcp_tool) never got the same protection: it only logged a warning, then ranextra_headers.update(hook_extra_headers)and clobbered the token.Change
_call_regular_mcp_toolinto a pure helper,_merge_hook_extra_headers_with_precedence(...), and give it the same precedence rule as thetools/listpath:Authorization— the serverauth_value(server_auth_header), the per-usermcp_auth_header, or a caller-supplied (non-static)Authorizationalready inextra_headers— is preserved; the hook'sAuthorizationis dropped (its other headers still apply).Authorization(server.static_headers), preserving existing behavior.Testing
Added unit tests for the precedence (OAuth via extra_headers /
server_auth_header/mcp_auth_headerpreserved; JWT applied when no credential; JWT still overrides static; non-Authorization hook headers always applied; case-insensitive). The existingtest_hook_headers_override_static_headersstill passes, confirming static-override behavior is unchanged.