Repository navigation
fix: preserve per-user OAuth Authorization over MCP signer JWT on tools/calls - #32251
RajanChavada wants to merge 1 commit into
Conversation
|
|
Greptile SummaryThis PR fixes a header-priority bug on the MCP
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the hook-header merge step and cannot affect callers that have no existing Authorization in extra_headers. The fix correctly mirrors the tools/list guard onto the tools/call path, uses case-insensitive header comparison, preserves all non-Authorization hook headers, and is covered by a mutation-verified regression test. The only gap is a missing debug log when the JWT is silently skipped — a minor observability concern with no correctness impact. No files require special attention; both changed files are straightforward.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Replaces the blanket extra_headers.update(hook_extra_headers) with a case-insensitive per-header loop that skips the hook's Authorization when one is already present in extra_headers, matching the existing tools/list guard logic. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py | Two existing tests updated to assert the correct fixed priority (existing Authorization wins); a new regression test for the delegate_auth_to_upstream / v1-path scenario from issue #31977 is added. All tests use mocks only — no real network calls. |
Reviews (1): Last reviewed commit: "fix: preserve per-user OAuth Authorizati..." | Re-trigger Greptile
| if isinstance(header, str) and header.lower() == "authorization": | ||
| if has_existing_authorization: | ||
| continue |
There was a problem hiding this comment.
When the hook's
Authorization is silently skipped because an existing one was already in extra_headers, nothing is logged. A debug-level message here would help operators understand why the signer JWT was not forwarded, matching the level of observability that the old warning provided for the inverse scenario.
| if isinstance(header, str) and header.lower() == "authorization": | |
| if has_existing_authorization: | |
| continue | |
| if isinstance(header, str) and header.lower() == "authorization": | |
| if has_existing_authorization: | |
| verbose_logger.debug( | |
| "MCPServerManager: hook_extra_headers 'Authorization' skipped — " | |
| "an existing Authorization header (per-user OAuth or static) takes precedence." | |
| ) | |
| continue |
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!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Relevant issues
Fixes #31977
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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).
Screenshots / Proof of Fix
pytest tests/.../test_mcp_hook_extra_headers.py→ 29 passed. Added aregression test on the v1
delegate_auth_to_upstreamoauth2 path (the scenario#31977 reports) asserting the resolved OAuth Authorization survives the signer
merge. Mutation-checked: reverting the fix re-fails the test.
Screenshot of error in the code:
Screenshot after the fix
Type
🐛 Bug Fix
Changes
On the MCP
tools/callpath, the signer's JWT unconditionally overwrote theAuthorization header via
extra_headers.update(hook_extra_headers), even when aper-user OAuth token had already been resolved into
extra_headers. Thetools/listpath already guards against this; this applies the same guard totools/call— the hook's non-Authorization headers still merge in.