Fix: Refresh auth context per Streamable HTTP request#1998
Draft
bgaidioz wants to merge 3 commits intomodelcontextprotocol:v1.xfrom
Draft
Fix: Refresh auth context per Streamable HTTP request#1998bgaidioz wants to merge 3 commits intomodelcontextprotocol:v1.xfrom
bgaidioz wants to merge 3 commits intomodelcontextprotocol:v1.xfrom
Conversation
913b3c5 to
96ae6b7
Compare
2 tasks
96ae6b7 to
e571c42
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changeset addresses issue #1250: after the MCP client refreshes its token and starts using the new one, server‑side tools still receive the old (expired) token from
get_access_token().CI failed on coverage for the new branch in
_handle_request. I added a small, low‑level unit test to cover it, but it’s fairly close to internals and I’m not sure this is the preferred approach. If there’s a better way to cover this path, please let me know what you recommend.Motivation and Context
We've hit the bug and found this matching GitHub issue. I provided test code to reproduce the bug. Eventually I noticed project
fastmcphit the same problem and a patch was committed.How Has This Been Tested?
AssertionError: assert 'token-a' == 'token-b').Breaking Changes
No.
Types of changes
Checklist
Additional context
My understanding: The context var is correct inside the ASGI request task where
AuthContextMiddlewareruns. The problem is that tool execution happens later in a different task. The context var value isn’t visible.StreamableHTTPSessionManageronly wraps the request in aServerMessageMetadataand writes it to aMemoryObjectSendStream. When that message is processed later, unless it's re-set, the context var isn’t the correct one.The fastmcp fix patches
get_access_token()to read the token from the current request (available via request metadata in the tool execution context). Which permits to avoid reading the stale context var. This change instead updatesauth_context_varfrom the current request’sscope["user"]inside_handle_request, so each message sees the latest token in the context var.